1214 Commits

Author	SHA1	Message	Date
Paul Nothaft	5fa04e647e	fix(categories): validate category name length instead of 500ing ... photo_categories.name is varchar(100). Neither the input nor the route checked length, so a 267-char name hit a raw Postgres "value too long", came back as a 500, and the form silently stayed open with no toast. Add isLength({ max: 100 }) to POST / and PUT /:id (the update route had the identical gap) so it returns the route family's normal 400 { errors: [...] } shape that the toast helper already renders, and maxLength={100} on the three category-name inputs (create + inline edit in CategoryManager, create in EventCategoryManager). Refs testplan REPORT.md #4 (Part 7.01).	2026-09-01 16:23:28 +02:00
Paul Nothaft	6f7aa59fad	fix(feedback): align word-filter severity vocabulary with the admin UI ... WordFilterManager.tsx sends low/moderate/high/block; the validator only accepted mild/moderate/severe, so 3 of the 4 UI levels 400'd with "Invalid severity level" -- including "block", the strongest advertised tier. Aligning isIn() alone would have made "block" accepted but semantically inert: feedbackModeration.js branches on 'severe'/'moderate', so "block" would fall through to the flag-only branch and behave as the weakest level. Map the UI vocabulary onto the existing outcomes instead, per the legend the UI itself renders: block -> reject, moderate/high -> needs approval, low -> flag only. 'severe' stays an accepted alias in the blocking predicate so any row written through the old validator (the field is optional, so a direct API caller could have stored one) keeps blocking. No data migration needed: the column is a bare varchar(20) default 'moderate' with no CHECK, no enum and no seed rows, and 'mild' already lands in the flag-only branch that 'low' now means. Refs testplan REPORT.md #2 (Part 3, J.11).	2026-09-01 16:23:22 +02:00
Paul Nothaft	e18ab0d842	fix(upload): enforce the configured per-file size limit on admin uploads ... getMaxFileSizeBytes() (general_max_file_size_mb, default 50MB) was only read by adminSettings.js to display the value. The admin upload routes streamed against a hardcoded ceiling instead, so the dropzone's "max. 50MB pro Datei" was never enforced: - adminPhotos.js POST /:eventId/upload -> 10GB hardcoded - adminPhotos.js POST /:eventId/chunked-upload/init -> 10GB hardcoded - v1/events.js POST /events/:id/photos -> 100MB hardcoded Resolve the cap per request (it is admin-configurable at runtime) and build the multer instance from it, mirroring what gallery.js and adminTransfers.js already do. The 400 names the configured limit and reuses gallery.js's exact error string so the frontend surfaces it identically. getMaxFileSizeBytes() clamps to MAX_ALLOWED_FILE_SIZE_MB, so the 10GB hard ceiling still bounds everything. gallery.js (guest upload) already enforced this correctly and is unchanged -- the report's claim that it did not is stale. Interpretation: general_max_file_size_mb is a single per-file cap with no photo/video split, and gallery.js already applies it blanket to guest video uploads, so admin video uploads now share it too. On a default install that means a 200MB video needs the setting raised first -- which is what the UI has been advertising all along. Refs testplan REPORT.md #1 (Part 7.06).	2026-09-01 16:23:04 +02:00
Paul Nothaft	884580d849	chore(main): release 3.122.2-beta.0 (#1258)	2026-09-01 07:30:45 +00:00
Paul Nothaft	7e6bfbecb2	chore(main): release 3.122.1-beta.0 (#1254)	2026-09-01 06:35:22 +00:00
Paul Nothaft	6938bad107	fix(events): apply the gallery password policy to publish and send-later (#1253) ... Both routes re-hash password_hash from a plaintext the admin re-types, and both validated it with nothing but express-validator's isLength({min:6}). So the configured complexity — moderate by default, meaning 8 chars plus upper, lower and a digit — governed creation and reset while these two doors accepted 'aaaaaa' and made it the live gallery password. Fixed for both at once, deliberately. Fixing only the newer send-later route would have made a quiet-publish password valid at publish time and rejected by send-later, leaving the admin unable to mail a gallery that is already live under exactly that password. Not an escalation — it needs admin auth plus events.edit, and such an admin could already set the same weak password through /publish. It is a policy gap: the UI promised a complexity level these two endpoints did not enforce. BEHAVIOUR CHANGE: an API-only consumer publishing with a sub-policy password now gets 400 with the same body shape event creation returns (error, details, score, feedback) instead of silently weakening the gallery. Two existing test fixtures had to change for the same reason — their intent was that the supplied password is carried and persisted, not that a weak one is accepted. Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-09-01 08:30:17 +02:00
Paul Nothaft	a35d2bad66	fix(archives): restore categories for original-filename archives on main too (#1252) ... main's #1240 landed the manifest lookup in its first form; the hardening that followed only ever reached stable, via #1243. So main still silently loses every category when restoring an archive written while general_use_original_filenames_for_downloads was on: archiveService names each ZIP entry after the ORIGINAL filename while the manifest stays keyed by the internal photos.filename, so the lookup misses every entry. Ported as one unit rather than piecemeal, since a third variant of this function helps nobody: - index by original_filename, and by sanitizeForZipEntry(original_filename) as the ZIP would actually have written it - two passes, canonical names claimed before any alias, so the result no longer depends on manifest iteration order (the archive query has no ORDER BY) - a name two rows both claim is dropped rather than guessed — including the canonical/alias clash, where which file the ZIP emitted depends on a naming mode the manifest does not record - globals count as existing, event-scoped rows win over them, and the global arm requires event_id IS NULL so one event's legacy row can't be adopted by another event's restore - an invented category is explicitly is_global false; the column defaults to TRUE, so a restore was leaking this event's naming into every gallery - categories resolve inside the !existingPhoto branch, so a restore that skips its inserts stops creating unused rows from stale manifest names - a duplicate category name is logged and resolved by lowest id instead of engine order main-only code is untouched: the face-data cleanup (#1074, #1132) and the uploaded_at toISOString fix both survive — stable still has the bare new Date() there, which is the documented Jest/SQLite landmine and worth a separate look. 15 tests, ported from #1243. Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-09-01 08:30:02 +02:00
Paul Nothaft	63f3fb4629	chore(main): release 3.122.0-beta.0 (#1251)	2026-09-01 06:27:11 +00:00
Paul Nothaft	1ef2b3c85b	feat(events): publish without notifying, and send the gallery email later (#1235) (#1241) ... * feat(events): publish without notifying, and send the gallery email later (#1235) Publishing queued the gallery_created email whenever any customer email existed, with no opt-out. A photographer working with a client who has no address yet — the Instagram-team case in discussion #1086 — had to type their OWN address into the required field, publish, receive the client-facing email themselves, and hand the link over by DM. Turning off `event_require_customer_email` is not the answer either: that is global, and the same photographer usually does collect addresses. Two halves, because a checkbox alone is only half a workflow: - `notify_customer` on publish, default TRUE. Absent means notify, so the v1 API, an older frontend and any script keep behaving exactly as before. When false the gallery goes live and nothing is queued — not the gallery_created email, not the assigned-customer-account notice, not WhatsApp. Publishing still logs activity and still fires the event.published webhook, because those describe a state change rather than a message to a customer. - POST /:id/send-gallery-email for an already-published gallery. Deliberately not restricted to galleries published quietly: re-sending is a normal thing to want (spam folder, wrong address since corrected) and refusing would push people to unpublish and republish, changing gallery state to work around a mail problem. Refused for a draft, whose link would not work yet, and for an event with no recipient. The email composition is now one helper shared by both, so an email sent a week later is identical to one sent at publish. UI: a checkbox in the publish dialog (checked by default, hidden when nobody would be notified anyway), and a "Send gallery email" action on published galleries that have a recipient. The password field follows the checkbox — unchecking it means nothing is being sent, so there is no plaintext to carry and no reason to demand it. EN + DE strings. 7 integration tests. Two fail without the change, verified by forcing notifyCustomer true and re-running; the rest pin the default, the draft and no-recipient refusals, and that a gallery with no recipient still publishes. * fix(events): make the publish dialog description follow the checkbox (#1235) Caught by screenshotting it. With "Send the gallery email now" unchecked, the paragraph above still read "...and sends the notification email to tina@example.com" while the control directly beneath it said nothing would be sent — the dialog contradicted itself at exactly the moment the admin is deciding whether anything goes out. It now reads "No email will be sent — you can send it later from this page." when the box is clear. EN + DE. * fix(events): close six gaps in publish-quietly found by external review (#1235) TWO CORRECTIONS TO MY OWN VERIFICATION FIRST. `npx tsc --noEmit` in frontend/ is a NO-OP — the root tsconfig is solution-style with references and no include, so it checks nothing. Every "tsc clean" I claimed on this branch came from that. The real check, `tsc -p tsconfig.app.json`, showed two TS2339s I had introduced: `event.host_email` does not exist on the frontend Event type, which the admin API normalises away. Both recipient checks now use `customer_email`. PASSWORD ON SEND-LATER. The action promised to send the link and password but always called the endpoint without one, so a protected gallery got the "(set at creation)" sentinel — unusable — and this is most needed right after a quiet publish, the path that never collects a password. New SendGalleryEmailDialog asks for it, same shape and reasoning as the publish dialog (#627). Galleries with no password skip the field. WHATSAPP-ONLY GALLERIES COULD NOT PUBLISH QUIETLY. willNotify ignored customer_phone, so a phone-only gallery hid the opt-out AND told the admin nothing would be sent — while publish queued the WhatsApp anyway. Phone now counts, with its own description line. ASSIGNED-ACCOUNT NOTICES COULD NOT BE SENT LATER. The dialog promised it; the endpoint rejected anything without an inline recipient. It now falls through to the same customer-account path publish uses. EDITORS COULD NOT SEE THE ACTION. The send button was nested inside the events.archive gate, so the default editor role — events.edit, no archive — never saw a button for an endpoint it is allowed to call. Separate gates now. DEAD LINKS. The endpoint only checked is_draft, so an archived, inactive or expired gallery would send a link the gallery middleware rejects. All three are refused with a reason. 9 backend tests (2 new), 22 across the event suites. eslint clean on every changed frontend file; crud.js keeps its 2 pre-existing errors. * fix(events): persist the send-later password, and fix a long-standing isGalleryPublic misuse (#1235) Round 2 of external review. THE EMAIL COULD CARRY A PASSWORD THE GALLERY REJECTS. The send-later dialog invites "or pick a new one", but the route queued that plaintext without touching password_hash — so the customer got credentials that do not open the gallery. Worse than the sentinel it replaced, because it looks usable. The route now hashes and persists first, exactly as publish does. isGalleryPublic TAKES A VALUE, NOT AN EVENT — and this is pre-existing. normalizeRequirePassword returns its default for anything that is not a boolean/number/string, so isGalleryPublic(event) is ALWAYS false and `requirePassword` was always true. The publish dialog on main has demanded a password for public galleries for exactly this reason. Both call sites now pass event.require_password. Fixing the older one alongside mine rather than leaving a broken copy one line above a fixed one. ASSIGNED-ACCOUNT GALLERIES HAD NO BUTTON. The route falls through to the customer-account notice when there is no inline email, and the publish dialog promises that notice can be sent later — but the button only appeared with a customer_email, making the promise unkeepable. WHATSAPP CLAIM SOFTENED. Publish only queues WhatsApp when the config exists and is enabled, which the dialog cannot see. It now says the customer is notified there "if WhatsApp is configured" rather than asserting a send. 10 backend tests (1 new, covering the rehash). eslint clean on every changed frontend file; crud.js keeps its 2 pre-existing errors. * fix(events): don't reset the password for an account-only notice, hide unusable actions (#1235) Round 3 of external review. The first is a harm my own round-2 fix introduced. PASSWORD RESET FOR NOTHING. Round 2 persisted the supplied password before knowing which mail would go out. For a protected gallery with no inline email but assigned accounts, the dialog still demands a password, the hash was rewritten, and then the fallback sent customer_gallery_assigned — which links to the customer portal and never mentions a password. Net effect: the live gallery password silently changed and everyone holding the old one was locked out, in exchange for nothing. It is now persisted only when the mail that carries it is actually being sent. BUTTONS THE BACKEND WOULD REFUSE. The send action rendered for expired and inactive galleries, and counted assigned accounts the endpoint filters out as inactive — walking the admin through a dialog to reach a generic error toast. The card now mirrors the endpoint's eligibility rules, and only active accounts count toward having a recipient. 11 backend tests (1 new, pinning that the hash is untouched on the account path), 24 across the event suites. tsc and eslint clean on the changed files. * fix(events): make the send-later action agree with what the endpoint will do Three findings from an external review round, all the same shape: the UI predicted the endpoint's behaviour and got it wrong. GET /admin/events/:id mapped customer_accounts without is_active, so the "only ACTIVE accounts count" filter in OverviewTab compared undefined and excluded nothing. A gallery whose only assignments were deactivated showed the send action, and the endpoint then filtered every recipient and returned 400. is_active is exposed now, and the count applies the same predicate the fallback uses — active AND holding an address. is_active is coerced through toBoolean rather than compared with === false. On the default SQLite backend it comes back as 0, and 0 === false is false, so an inactive gallery kept offering a send that parseBooleanInput then rejected. Same class as #1028. The password prompt is gated on there being an inline recipient. With no customer_email the backend takes the account fallback, which sends customer_gallery_assigned — a portal link that never mentions a password — and deliberately skips the rehash. Asking for one there blocked the send behind a six-character value nothing consumes, and the dialog's promise that it would be rehashed was false. Frontend suite: 291 passed. tsc and eslint clean. * fix(events): don't mail a portal link to a customer who cannot sign in Round-2 finding from the external review. A passive customer — created directly and never invited — is an active account with a real address whose password_hash IS NULL. The account fallback happily mailed it customer_gallery_assigned, which links to /customer/dashboard, and customerAuth rejects login without a hash: the link goes to a door that will not open. Worse than failing, the route counted it and reported success, so the admin believed the customer had been told. getAssignmentsForEvent now derives can_sign_in (the predicate, never the hash) and the three call sites share one canReceiveGalleryNotice helper — publish, send-later, and the payload the UI predicts from all have to agree or the button appears and then 400s. The UI mirrors it. Sending passive customers an invitation instead of skipping them is the better product answer, and a separate feature. Refusing visibly beats a silent non-delivery in the meantime. Test asserts the refusal; it fails without the can_sign_in arm. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-09-01 08:18:00 +02:00
peipeimo	bb2f709fdd	fix: single-photo gallery downloads 404 on S3 storage backends (#1048) ... * fix(gallery): route single-photo downloads through the storage backend The route resolved a local filesystem path unconditionally and handed it to res.sendFile. On an S3/R2 deployment managed photos are never on local disk, so every per-photo download failed — while download-all and secure-images worked, because they already went through getStorage(). That asymmetry is why it went unnoticed: the gallery looks healthy until a guest clicks the download button on one photo. Measured rather than assumed: because sendFile is called WITH a callback, Express does not send a response when the file is missing and the callback only logs. The request does not 404, it hangs until the client gives up. The new tests pin this — all five backend-path cases time out against the previous implementation. Two existing pieces do the work, so this mostly deletes code: - renderPhotoForDownload (#858) already owns resize-then-watermark ordering and the storage fetch, and the zip builders in this same file already use it. The inline duplicate of that logic goes. - the pass-through case branches on storage.kind(). Local disk keeps res.sendFile: it emits Content-Length, Accept-Ranges, ETag and Last-Modified and answers Range with a 206, and sharing one bare stream.pipe(res) with S3 would silently drop all of it — a resumed download would append a second full body onto the partial file. On S3 the parts that matter for a download are reproduced via stat() and getRange(). Ranges are parsed defensively; an unchecked parse yields NaN bounds and a 206 with a nonsense Content-Range, which corrupts a resumed download rather than failing it. Malformed or unsatisfiable ranges fall back to a 200. The pre-stream 404s now run before any image header is staged, so the error goes out as JSON instead of a .jpg attachment containing JSON. Co-authored-by: peipeimo <peipeimo@users.noreply.github.com> * fix(gallery): open the stream before staging download headers, honour If-Range Both from an external review round on this PR. stat() succeeding does not mean get() will — a concurrent delete or replace, or a transient backend error, lands between them. The fetch was awaited AFTER the headers went out, so: - the range branch had already called writeHead(206), leaving the outer catch nothing to do but throw ERR_HTTP_HEADERS_SENT. In practice the request hangs: the new regression test sat for the full 120s jest timeout against the previous code instead of returning. - the full branch would have sent its 500 JSON underneath the staged image/jpeg attachment headers — a .jpg file full of JSON, which is the exact failure this PR set out to stop doing on the 404 paths. Opening the stream first also lets a vanished object answer 404 and a transient failure answer 500, instead of both surfacing as a broken body. If-Range: emitting Last-Modified without honouring the validator built from it is the dangerous half of the feature. A client resuming after the object was replaced — the watcher re-importing a swapped file, an admin re-upload — would get 206 from the NEW bytes and splice two versions into one corrupt file. A validator that does not match now falls back to a full 200. 4 new tests; 3 of them fail against the previous commit, the fourth is the matching-validator control that must keep returning 206. * fix(gallery): HEAD without egress, classify render failures, stage 206 headers Round-2 findings from the external reviewer. Express routes HEAD through this GET handler and Node discards the body, but the pipe still drains the whole object out of S3 first — a metadata probe from a download manager cost a full transfer in egress and latency. Everything a HEAD needs is already in stat(). renderPhotoForDownload rejections were all reported as 404. It can equally fail because getToFile timed out, tmp filled up, or sharp died; calling that "photo not found" misleads the guest and hides the incident from us. Now classified the same way the pass-through branch already does. The 206 path uses status()+set() instead of writeHead(). writeHead commits the response immediately, so a stream that resolved and then errored before its first chunk left pipeStreamToResponse able only to destroy the connection. Staged headers flush on the first body write, so an error at byte zero now returns a clean retryable status with keep-alive intact. Credit to the reviewer for the correction — I had assumed deferring the commit required buffering. Writing the test for that surfaced one more: pipeStreamToResponse cleared Content-Type, Content-Length, ETag and Content-Disposition but not the range headers, so the 500 went out still advertising Content-Range: bytes 0-9/40 — telling a resuming client the error body IS the partial content. Not taken: binding response metadata to a fetched object version. That needs an ETag/versionId on the storage abstraction and conditional GETs in both adapters; the reviewer agreed it belongs in its own PR rather than blocking this one. Backend suites: 485 passed. * fix(gallery): answer HEAD before the counters and the render Round-3 finding. The HEAD short-circuit was inside the storage branch, which sits below both the download_count increment / access_logs insert and renderPhotoForDownload — so a download manager's metadata probe was recorded as a real download, and on a watermarked or resized gallery it also pulled the original from S3 and ran sharp over it to build a body Node then throws away. HEAD now leaves the handler right after the access checks, with no side effects and no bytes read. Content-Length is included only when the photo ships untransformed and the size is readable from stat(); a watermark or resize changes the length and the only way to learn the new one is to do the work this branch exists to avoid. HEAD may omit it. Not taken, again: binding the read to the statted object version. The reviewer already agreed in a follow-up that it needs an ETag/versionId on the storage abstraction plus conditional GETs in both adapters, and belongs in its own PR. Re-raising it does not change that. Tests assert the probe moves neither download_count nor access_logs. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local> Co-authored-by: peipeimo <peipeimo@users.noreply.github.com>	2026-09-01 08:17:32 +02:00
peipeimo	202c553a08	fix(events): delete stored objects when cascading an event delete (#1051) ... * fix(events): delete stored objects when cascading an event delete * fix(events): sweep watermarks and the archive zip on cascade delete too Two more objects in the same class as the originals: both are written through the storage backend, both were only ever removed with fs.unlink, so both outlive the event on S3. - photo.watermark_path — a canonical key, deleted via getStorage() on the single-photo path (watermarkService.deleteWatermarkFile) and on archive (archiveService.js:227). The cascade neither selected nor removed it. - event.archive_path — written by storage.putFromFile (archiveService.js:160) and typically the largest single object an event owns. event.hero_logo_path is deliberately NOT included: multer writes logos to local disk with diskStorage regardless of backend (adminEvents/logo.js:19-28), so they are never bucket objects and the existing fs.unlink is correct. Collect into a Set — an unresized gallery can carry one object in both hero_path and preview_path, and the second delete would log a spurious failure. * fix(events): sweep the download caches, and delete objects concurrently Both from an external review round on this PR. The download caches are the subtle case: the pre-built "Download All" zip (events.download_zip_path) and one zip per custom-resolution download job (download_jobs.zip_path) both live under events/active/{slug}/.download-cache/. On local disk the recursive fs.rm already covered them, which is exactly why they were easy to miss — on S3 that prefix is not a directory, nothing covered them, and both are gallery-sized. downloadZipService exposes a cleanup() documented as "used on event deletion" that the cascade never called. The job rows are read before the transaction for the same reason the photo rows are: download_jobs.event_id is ON DELETE CASCADE, so on Postgres they vanish with the event and take their keys with them. Guarded with hasTable so a pre-#173 install doesn't abort the delete. Deletes now run through a bounded pool instead of one await per key. A 400-photo gallery owns ~1600 objects once derived tiers are counted, and that many sequential DeleteObject round trips runs to minutes — long enough for a proxy to time the request out AFTER the commit, leaving the event deleted and the sweep half-finished. A pool rather than Promise.all over every key, so the fan-out can't exhaust the S3 client's connection pool. * fix(events): never delete a derivative another gallery still uses Round-2 findings from the external reviewer. Canonical thumbnail/hero/preview keys are not event-scoped: the basename is the photo's filename (imageProcessor passes no outputBasename for managed photos, so the key is thumbnails/thumb_w300_<filename>), and filenames are not unique across events — the responsive-tier code says so in as many words, which is why THOSE keys carry a p{id}_ prefix. A legacy gallery can therefore share a canonical derivative with a photo in another event, and deleting it here blanked a surviving gallery's tile. Derived keys are now checked against photos outside this event and anything still referenced is left alone; if the check itself fails, every derivative is kept. An orphan costs storage, a deleted derivative costs someone else's gallery. Originals need no check — their keys embed the slug. Also cancel any in-flight or debounced Download All build before snapshotting paths. A builder that started before the delete would otherwise upload a gallery-sized zip after the sweep and write its path onto a row that no longer exists, orphaning it permanently. downloadZipService.cleanup() is the service's own entry point for this and does all three things: bumps the version so an in-flight build discards its result, clears the debounce so nothing rebuilds for a deleted event, and removes the current object. * revert(events): drop the Download All build cancellation Reverted for the same reason as on the stable twin, where it was caught: downloadZipService.cleanup() reaches getStorage() through _cleanup(), so where the S3 backend is configured but unreachable every cascade delete pays the adapter's retry backoff. On stable that took the backend CI job from ~2 minutes to past its 10-minute budget, twice, reproducibly. This branch's suite happened not to trip it, but the same cost lands in the request path of a real delete — and the twins have to carry the same code. The race it addressed is narrow and costs one orphaned zip; documented as a follow-up instead. The shared-derivative guard from the same review round stays — that one prevented deleting a surviving gallery's thumbnail. --------- Co-authored-by: Peifu Mo <peipeimo@Peifus-MacBook-Pro.local> Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-09-01 08:17:04 +02:00
Paul Nothaft	bdeb5a2151	chore(main): release 3.121.4-beta.0 (#1249)	2026-09-01 06:10:24 +00:00
peipeimo	4f352dec39	fix(auth): treat zxcvbn suggestions as advice, not blocking errors (#1050) ... validatePassword() appended zxcvbn's feedback.suggestions to the errors array unconditionally, and validity is errors.length === 0 — so any password that merely earned a suggestion was rejected even when it satisfied every configured rule. The effective policy was stricter than the configured complexity level and invisible to the admin. Suggestions now surface only alongside a real strength failure. They stay available to callers in result.feedback.suggestions, so a UI can still show them as guidance while typing. The weak-password fixture is assembled from parts rather than inlined: an 8-char alphanumeric literal next to validatePassword( reads as a hardcoded credential to the required GitGuardian check. Both fixtures pin their zxcvbn score — the compliant one is load-bearing at exactly the moderate minimum (2), and a future zxcvbn bump promoting it to 3 would leave the test green while no longer covering the bug. Co-authored-by: Peifu Mo <peipeimo@users.noreply.github.com>	2026-09-01 08:05:42 +02:00
Paul Nothaft	c7ce79afb1	chore(main): release 3.121.3-beta.0 (#1242)	2026-08-30 15:07:47 +00:00
Marian	0d340f4e81	fix(archives): take the restored category from the manifest (#1240) ... * fix(archives): take the restored category from the manifest The archive writer already persists `category_name` per photo in photos_manifest.json — that is why the manifest exists, and the comment above it says so: "(and category linkage) can't be derived from the extracted files alone". The restore route then read only `original_filename` out of it and kept deriving the category from the ZIP's first path segment. Archives store photos exactly as they sit on disk, so an event whose photos live in the gallery root produces a FLAT zip. `path.dirname()` is '.' for every entry, no category is resolved, and every restored photo lands with `category_id = null` — silently, behind a 200. Seen on a real restore: 596 photos back, 0 with a category, while the nine category rows sat untouched in the table. Now the manifest is the source of truth and the first path segment is the fallback, so foldered archives and legacy archives without a manifest behave exactly as before. The find-or-create is pulled into `resolveCategoryId` so both paths share it and each name is resolved once per restore. Tests: __tests__/integration/adminArchives.restoreCategories.test.js builds real ZIPs (flat with manifest, flat with an existing category row, foldered without manifest) and drives POST /:id/restore. Without this change the two manifest cases fail and the foldered one passes — the fallback is unchanged. * fix(archives): let the manifest be authoritative when it says "no category" Review follow-up on #1240, pushed with the author's agreement. The manifest won for "category X" but not for "none": an entry with a null category_name fell through to the directory fallback, so a photo the archive recorded as uncategorized came back filed under a category anyway. That matters because the directory is not a category. Archive entry names are the storage key minus `events/active/{slug}`, and that layout is `individual/{filename}` / `collages/{filename}` — categories have never been directories there. Reading the first path segment on a real archive invents categories literally named "individual" and "collages", so the fallback was overriding an accurate record with a junk one. The fallback is now confined to photos with NO manifest entry at all: archives written before the manifest existed, where the directory is the only signal left and inventing those names still beats losing every category. Tests: the legacy case now uses `individual/`, the shape a real archive actually has, instead of a category-shaped folder no archive produces — so it documents what the fallback really does. Plus a new case pinning that a manifest saying uncategorized leaves the photo uncategorized and creates no category row. It fails without this change; the legacy fallback keeps passing. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-30 17:02:14 +02:00
Paul Nothaft	44a8c416c7	refactor(email): stop reading the webhook response body at all (#1225) (#1239) ... The transport carried 41 lines of bounded-read-with-deadline to recover a messageId a receiver MIGHT return. That value is only ever logged — nothing persists it, there is no email_queue.message_id column — and the code to get it produced two of the last four review findings: the size cap made a DELIVERED message retry (axios throws while reading), and the missing deadline let an unclosed stream hang the queue and resend. Not reading the body is how that whole class stops being reachable rather than defended against. responseType 'stream' still keeps axios from buffering; the stream is destroyed immediately and the id is synthesised as before. The status was always the delivery verdict, and it is known before any of this. An 'error' listener goes on before destroy(): destroy can emit on a socket-backed stream, and an unhandled 'error' on a stream throws — which would have turned a receiver's teardown into a failed send. Net: -45 lines of service code, one fewer constant, one fewer test seam, and three of the hardest cases in the suite replaced by two simpler ones. 23 tests, 63 across the email suites, eslint clean. Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-29 13:06:19 +02:00
Paul Nothaft	89e8e41c40	chore(main): release 3.121.2-beta.0 (#1238)	2026-08-29 10:27:50 +00:00
Paul Nothaft	6ca8baab23	fix(watcher): stop re-importing a photo whose file was replaced (#1226) (#1237) ... The existence check matched on filename OR path. replacePhoto regenerates both — a fresh generated filename and a fresh managed path — so a watched-folder photo that had its file replaced stopped matching either arm. The original is still sitting in the watched folder, so the next sweep imported it again and the gallery ended up holding the delivered edit AND the untouched original: the same duplicate shape external_relpath prevents for reference galleries. source_filename is now a third arm. It is the stable key here — written once at ingest by this same path and preserved across a replace by design. Rows predating migration 193 are covered by its backfill: COALESCE(original_filename, filename), and this path never wrote original_filename, so for watcher rows that resolves to the basename being compared. The query is lifted into an exported findExistingPhoto() so the test drives it rather than a copy — the thing under test IS the query, so a query-builder mock would only assert that knex was called the way the test expects. Predates the Lightroom round-trip and applies to the admin replace path too; it became reachable when #1165 brought watcher galleries into round-trip scope. Six tests against a real SQLite database. The load-bearing one fails without the change, verified by removing the arm and re-running; the other five pin what must not move — filename and path matching, the pre-193 backfill shape, a genuinely new file, and event scoping. Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-29 12:22:50 +02:00
Paul Nothaft	064b1bcb14	chore(main): release 3.121.1-beta.0 (#1236)	2026-08-29 10:21:22 +00:00
Paul Nothaft	4105c099c9	test(external): make the fold-collision guard test the real code (#745 follow-up) (#1234) ... The regression test added with #1165 re-implemented the claim ordering and the claim loop inside the test file and asserted against its own copy. It never required externalRelpathFold, so changing the real sort left it green — a guard against silently deleting a client's delivered edit that guarded nothing. The ordering is now a named, exported claimOrderFor() and the test drives it. Verified by sabotage: replacing the comparator with `return 0` fails the test, where before it passed. Three cases added while the seam existed: the managed row wins from BOTH input orders (the original bug was that the survivor was whichever came first, so one order proves nothing), the sort is stable for rows of the same kind, and it does not mutate the caller's array. No behaviour change — the comparator is byte-identical, only lifted out. Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-29 12:15:31 +02:00
Paul Nothaft	0d41fe5bf1	fix(email): keep the webhook payload out of the logs, and bound the response read (#1225) (#1233) ... Round 4 of external review, on the merged commit. Both findings are consequences of the round-3 streaming change, which is exactly why the round was worth running. An AxiosError carries the request it failed on: `config.data` is the ENTIRE serialised message, base64 attachments included, and `config.headers` holds the signature. emailProcessor logs the error object and winston serialises it, so a DNS blip or a refused connection wrote password-reset links, guest recovery codes and multi-megabyte invoices into combined.log — verified against axios rather than assumed. Every rejection is now caught and replaced with a message-and-code-only error, so nothing downstream can serialise the request back out of it. readBounded had no deadline. axios' `timeout` covers the response HEADERS, and with responseType 'stream' it has already resolved by the time the body is read — so a receiver that answered 2xx and never closed its body left the await hanging, the queue row stayed pending, and the next processor pass sent the same message again. An unclosed stream was duplicate email. There is now a 10s wall clock that destroys the stream, with the timer unref'd so a hung read cannot hold the process open at exit. 23 transport tests (2 new, both failing without these fixes), 63 across the email suites, eslint clean. Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-29 12:15:25 +02:00
Paul Nothaft	25a7e64951	chore(main): release 3.121.0-beta.0 (#1232)	2026-08-29 09:16:49 +00:00
Paul Nothaft	d62407f431	feat(email): webhook transport as an alternative to SMTP (#1225) (#1231) ... Setting EMAIL_WEBHOOK_URL makes PicPeak stop sending mail itself and POST each composed message as JSON instead, for something downstream (n8n, Make, a self-hosted relay) to deliver. Unset, every SMTP path is unchanged. Settles the four things #1225 left open: - SSRF: the URL goes through the same DNS-resolving check the outbound webhook worker uses, before every send. Private receivers are opt-in. - Transport security: https is required for anything leaving the machine. The HMAC proves who sent the body, not who can read it, and these bodies carry password-reset links and guest recovery codes. The private-network opt-in doubles as the plaintext opt-in. - Authentication: EMAIL_WEBHOOK_SECRET is required and signs the body as X-PicPeak-Signature, the same scheme as gallery webhooks. A URL without a secret leaves the transport OFF and says so once. - Attachments: carried as base64, not dropped. Oversized ones fail and stay queued rather than arriving without the invoice. Configuration is environment-only on purpose: this redirects every outbound message including password resets, so it must not be changeable from a compromised admin session. Three wiring details decide whether it works at all: docker-compose.yml declares an explicit environment block, so the vars had to be forwarded there; a fresh webhook-only install has no email_configs row (migration 001 seeds it only when SMTP_HOST is set), so the From identity falls back to EMAIL_FROM; and processEmailQueue used to return early when SMTP could not initialise, which would have left the queue permanently unprocessed. guestRecoveryService and the admin test-email endpoint were bypassing the transport — the first dereferenced a null transporter, the second told webhook-only admins to go configure SMTP. emailIntakeService deliberately stays on SMTP: it round-trips a specific mailbox's own credentials. Response handling is streamed and read bounded by hand rather than capped via axios: maxContentLength throws while reading, so a receiver that delivered the mail and then echoed a large body would have been recorded as failed and the message sent again. Note: docker-compose.dev.yml is gitignored and local-only, so the equivalent entries there are not part of this change. docker-compose.production.yml needs none — it passes .env through with env_file. Three rounds of external review; 21 transport tests, 61 across the email suites.	2026-08-29 11:10:32 +02:00
Paul Nothaft	f4c054a661	fix(export): name the camera master in photo exports, not the delivered render (#1229) (#1230) ... #1165 added photos.source_filename to this service's select, with a comment saying it was there so the Lightroom round-trip could still match after a re-upload — and then nothing read it. Every output path still used original_filename, which is overwritten the first time an edited render is uploaded over a proof (#745). So after a round-trip the exports named the render. Each of these formats exists to help a photographer find the master on disk, and the render's name does not. The XMP case is the sharpest: the sidecar is written next to a RAW master, so a wrongly-named one is never associated with it. Two helpers, because the sites want different things when nothing is known: cameraName() source_filename || original_filename || null cameraFilenameOrStored() the above, else the stored name The dedicated `original_filename` fields (CSV column, JSON key) keep reporting blank/null when unrecorded — echoing the sanitized stored name there would invite a match against a file that does not exist under it. The places that must emit some name (text list, CSV filename cell, XMP sidecar) fall back to the stored one, as they did before. filename_format='stored' is untouched, and rows with no source_filename still resolve to original_filename, so nothing moves for installs that have never run a replacement. Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-29 11:10:05 +02:00
Paul Nothaft	4f684eb482	fix(feedback): name the camera original in the exports, not just the stored file (#1224) (#1228) ... Both feedback exports carried only `photos.filename` — the sanitized stored name (`wedding-smith_individual_1755892345.jpg`). Acting on client picks means finding the master on disk, and that name matches nothing in a Lightroom catalog, so the export could not be joined to anything. Adds the camera-original name to the long and pivot shapes, and by extension to the archive's feedback_data.csv/.json, which reuse the same query. COALESCE(source_filename, original_filename), not original_filename alone: the latter is overwritten the first time an edited render is uploaded over a proof (#745), so an export taken after a round-trip would name the render rather than the master and silently stop matching. source_filename is written once at ingest and survives a replace by design (migration 193). That case is the load-bearing test. Aliased to `original_filename` — the name the sibling photo export already uses for this column, and the question the reader is asking. Left empty when neither is known rather than echoing the stored name: blank reads as "no match possible", where repeating the sanitized name invites a match attempt against a file that does not exist under it. The column is added, not swapped: `filename` is untouched, so anything reading the old column keeps working. Reported by the 8digit/picpeak fork, which has carried a narrower version of this patch (original_filename only) across rebases. Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-29 11:09:59 +02:00
Paul Nothaft	a8a8b7cc64	chore(main): release 3.120.0-beta.0 (#1227)	2026-08-28 13:50:35 +00:00
Luca	8db8527f9e	feat(api): Lightroom round-trip — read proofing marks, put finished edits back (#745) (#1165) ... * feat(api): Lightroom round-trip — read marks, put edits back (#745) Gets a client's proofing verdict into a desktop catalogue and a finished edit back over its proof, without anyone re-matching files by hand. Three parts: **Keep the camera filename.** photos.original_filename is the only carrier of `IMG_1234.JPG` — the stored filename is rewritten by generatePhotoFilename. But replacePhoto() overwrites original_filename with whatever name the new file arrives under, so the first re-upload of a renamed render destroys the key the NEXT round-trip needs. Migration 185 adds photos.source_filename, written once at ingest and never touched by a replace, backfilled from original_filename so existing galleries can still match on their first pass. The backfill sits outside the column guard and keys on whereNull, so a run that dies partway self-heals instead of leaving half the rows empty forever. **Read the marks.** GET /api/v1/events/:id/photos returns each photo with its client colour tallies, the caller's own marks, and a merged colour + rating. Guards copied from the sibling upload route (apiTokenAuth + read scope + photos.view + requireEventOwnership). Filters: marked_only, mark_source, color_labels, my_color_labels, min_rating, my_min_rating. The route filters to a page of ids with PhotoFilterBuilder, then enriches just those through photoExportService.getPhotosWithFeedback — the two halves already existed and neither does both, and going id-first keeps the per-colour tally query bounded by page size. services/markMerge.js decides how three possible opinions (guest colours, guest star average, the photographer's own row in photo_admin_marks) collapse into the one colour and one rating Lightroom has room for. Colour goes to the photographer on a tie — one deliberate choice beats an aggregate a tie-break already had to guess at. Rating takes the max, because a rating is a magnitude and losing the higher one quietly demotes a photo somebody rated highly. Its roundRating matches XmpGenerator.mapRating exactly so the API and an XMP sidecar can never disagree about how many stars a photo has. **Put the edit back.** POST /api/v1/events/:id/photos accepts an optional replaces_photo_id and routes to the existing replacePhoto(), preserving the photo's id, feedback, comments and position. The plugin stores the picpeak id on the catalogue photo, so the id survives the editor renaming the render — which makes it the reliable key, not the filename. Scoped to the event in the URL: a token inherits its owner's powers across every event they can see, so an unscoped id would let one gallery overwrite another's photo. For renders whose RAW never went through the plugin, findReplacementCandidate gains an opt-in number_token mode matching on the trailing digit run. Deliberately the LONGEST run and never a fixed last-N slice: multi-camera shoots disambiguate by prefixing the camera index into the number (cam11234.jpg / cam21234.jpg), and a last-4 slice reads 1234 from both bodies and reintroduces exactly the collision the prefix removes. Ambiguity is refused, never guessed. Also drops the multer temp file on the two new early returns — this route only unlinks in its catch block. * refactor(api): one rating-rounding rule, and apply match_mode where it counts Three things the pre-review pass turned up on the round-trip work: - `match_mode` reached the photo-cap pre-count but not the loop that actually picks the replacement target, so asking for `number_token` would have been counted and then quietly ignored. Both call sites now take it. - `number_token` matching read `select('*')` over every photo in the event to compare one digit run. It now reads the three columns the match needs and re-reads the single winner in full, so a 5000-photo event doesn't pull 5000 full rows through memory to answer one question. - `XmpGenerator.mapRating` and `markMerge.roundRating` were the same five thresholds written twice — the second way to do one thing that drifts the moment either is touched. The thresholds now live in markMerge and the generator delegates, which is what keeps a sidecar and the v1 API from ever disagreeing about a photo's star count. * fix(api): keep the new route in the generated OpenAPI spec The `color_labels` description carried an inline JSON example. In an unquoted YAML scalar `{ "green": 2 }` parses as a flow mapping, so swagger-jsdoc threw YAMLSemanticError and dropped the WHOLE route from the spec — visible only as a warning on boot, with the route still serving normally, which is exactly the kind of failure that survives to release. Found by booting a real instance rather than by reading the diff. * fix(api): close the four blockers from review on #1165 1. Replacing an external photo silently kept serving the old file. resolvePhotoStorageKey gives photo.source_origin precedence and returns null for 'reference'/'external', so the edit was uploaded, the row updated and 200 returned while every viewer kept getting the untouched NAS original and the upload sat orphaned. replacePhoto now repoints the row to managed and clears external_relpath. The file on the share is never touched — this moves the pointer, not the data. 2. Every replacement leaked its temp file. putFromFile COPIES on local and uploads on S3; neither consumes the source, and replacePhoto never unlinked it — while the v1 route had disabled its own cleanup on the belief that replacePhoto moved the file. Cleanup now lives in replacePhoto, which closes the admin path too (adminPhotos only unlinks in its new-files branch, so replaced files leaked there as well). The v1 route also unlinks on the FAILURE path, which returned before any cleanup ran. 3. The download-all ZIP is invalidated after a replacement, as adminPhotos.js already does. Without it guests kept downloading the pre-edit photo indefinitely, which defeats the point of the feature. 4. The round-trip could not see reference or watcher galleries at all. fileWatcher and adminExternalMedia never set original_filename — the camera name lives in `filename` for those rows — so the backfill and the GET fallback both produced NULL for exactly the galleries most likely to be driven from Lightroom. The backfill now COALESCEs, both ingest paths set source_filename, and the GET falls back to filename. Concerns: - number_token no longer reads every photo row in the event per file. A LIKE on the digit run narrows the candidate set in SQL first; the exact trailing-run check still decides, so semantics are unchanged. The token is a regex-extracted digit run, so it cannot carry a wildcard. - The replacement's activity entry is scoped to event.id instead of null. The dashboard feed excludes NULL-event rows for scoped callers (GHSA-jhcf), so it was vanishing from the audit trail of the photographer who owns the event. Nit: dropped the unused higherPriorityColor export from markMerge. Three regression tests cover the external repoint, the temp cleanup and the COALESCE backfill. 21/21 pass. * chore(migrations): renumber 185 -> 193 after gallery-folders landed 185_add_category_is_folder.js merged to main while this was in review, so the number the PR reserved is taken and main is now at 192. Knex keys on filename rather than the prefix, so both would have run — but picpeakImportService guards restores with migrationOrder(), which parses that prefix, and two files answering 185 make the forward-only check pass a backup onto a schema missing its columns. Renumbered with every reference: the header comment, the test that requires the path, and the four call-site comments that cite it. The 'migration 182' reference inside it is the colour-labels migration and is unrelated; gallery.js:1134 cites upstream's 185 and is untouched. * fix(api): keep external_relpath when a replacement converts the row The external-photo blocker fix cleared external_relpath along with flipping source_origin, which closed one hole and opened another. adminExternalMedia dedupes a re-scan on (event_id, external_relpath) — routes/adminExternalMedia.js:195 — and migration 186 puts a unique index on exactly that pair. With the column nulled, the next scan of the share would not recognise the NAS original as already imported and would insert it again, so the gallery would end up holding both the edit and a fresh copy of the file it replaced. Only source_origin needs to change: it is what resolvePhotoStorageKey keys on, and every other consumer of external_relpath reads the two together and lets source_origin decide. The stale relpath on a managed row is inert for resolution and still correct as a dedupe key. Test updated to assert the value is kept rather than cleared. * fix(uploads): say when exiftool is missing instead of blaming the RAW A server without exiftool reported `No usable embedded preview in RAW file X.CR3: spawn exiftool ENOENT` for every RAW upload. The headline describes a corrupt photo; the actual cause is a package that was never installed, demoted to a trailing detail. It sends people hunting through their camera files. Hit while testing the Lightroom round-trip (#745): an export of RAW originals failed 11 times with that message, and the file was fine. RAW upload is the only feature that needs exiftool, so an install can be missing it indefinitely and only find out when someone uploads a CR3 — which makes the wording the whole diagnosis. ENOENT now produces a message naming the dependency and the install command for Debian/Alpine/macOS, and breaks out of the tag loop instead of spawning the same missing binary twice more to report the last failure as if it described the photo. A genuinely preview-less RAW still gets the original message. Verified both paths by making exiftool unreachable via PATH rather than mocking: missing tool and unreadable file now report differently. * fix(external): a delivered edit must win a relpath-fold collision Follow-up to keeping external_relpath on a replaced photo. Keeping it is what lets adminExternalMedia still dedupe the folder re-scan, but it also leaves the row inside externalRelpathFold's sweep — and that sweep does not merely rewrite paths, it DELETES collision losers via externalPhotoDedupe. The survivor was whichever row happened to be claimed first, which is iteration order. So a replaced photo — source_origin 'managed', holding the edit the photographer just delivered — could be deleted in favour of the untouched camera original sitting next to it on the share. Managed rows now claim first and therefore survive. The external row that loses is the recoverable one: it is still on the share and a re-scan re-imports it. The edit is not recoverable. Note this is deliberately NOT the "skip managed rows in the fold" shape suggested in review. Skipping would leave those rows holding a base-relative path while every other row moved to root-relative, so the scanner — which computes root-relative — would stop matching them and import the camera original again as a duplicate. That is the exact bug keeping external_relpath exists to prevent, reintroduced through a different door. Rebasing them and protecting them from deletion keeps both properties.	2026-08-28 15:45:56 +02:00
Paul Nothaft	1d88fa01ce	chore(main): release 3.119.0-beta.0 (#1223)	2026-08-28 06:41:39 +00:00
Paul Nothaft	5c85e0c0e4	fix(guests): surface duplicate guest registrations, and stop making so many (#1210) (#1216) ... * fix(guests): surface duplicate guest registrations, and stop making so many (#1210) Guest registration always inserts. A client whose token expired — or who opens the gallery on a second device — becomes a new gallery_guests row, and their likes and favourites split across the copies. The photographer's 'final selection' is then only trustworthy if somebody notices two Tinas with half the picks each. Two halves, neither of which touches the registration path. **Say which rows are the same person.** Merging already worked, endpoint and UI both; nothing said WHICH rows to merge. The guests list now marks each row with the others sharing its email and returns a count for the banner, and the admin list offers the group straight to the merge mode that already exists. Case-folded and trimmed, because the same person types Tina@ one day and tina@ the next and both read as distinct rows. Email only — two guests called Anna are not evidence of anything, and rows without an email are not grouped at all since require_name_email is off by default and a shared link produces plenty of them. It preselects rather than merges: which row survives decides the name and verification state the merged guest keeps, and that is the admin's call. **Create fewer of them.** The guest token was 24h and every call site took that default, so even the same browser lost its identity after a day of inactivity. Now 30 days, GUEST_TOKEN_TTL to override. A guest token is scoped to one event, carries no admin capability, and the gallery is already behind whatever protects it — 30 days is the shape of a real proofing cycle. Deliberately NOT done: reusing a guest row when a typed email matches, which the report suggests first. It would let anyone who knows an address inherit that person's identity and selections, and answering differently for a known email would leak which addresses are in the gallery — the thing /guest/recover already goes out of its way to avoid. Prevention at the entry path needs the verification round-trip, which is a separate decision about friction. 13 tests; 8 of the 9 backend ones fail without the change. The frontend ones caught a real bug while being written — the new useMemo sat after the loading early-return, so the hook count changed between renders. * fix(guests): merge must not strand a pending invite (#1210) Three findings from external review of #1216. **A merge could kill an emailed invite link.** Creating an invite inserts a real gallery_guests row, so an admin who pre-mints one and then sees the guest self-register has two rows sharing an email — which this feature now points out and offers to merge. Redemption resolves guest_invites.guest_id with is_deleted: false, so merging soft-deleted the row the link pointed at: the client got 404 guest_missing while the invite dialog still showed the invite as Pending. Nothing anywhere said the link was dead. Unredeemed, unrevoked invites now move to the survivor first. Spent ones stay put — a redeemed invite records who redeemed what, and retargeting it would rewrite that. **The preselection silently chose the survivor.** performMerge keeps mergeSelection[0], and the group was handed over in API order, which is newest-first — so Review then Merge discarded an older, email-verified row holding most of the picks in favour of a fresh re-registration. The proposal is now ordered deliberately: verified first, then whoever holds the most feedback, then the oldest. Still only a proposal, and the confirmation now names the survivor by email as well as name, because duplicates share a name and 'Merge 2 guests into Tina?' said nothing. **duplicate_of was quadratic.** Every row carried the other n-1 ids, so a group of n serialised n² of them — and nothing consumed the list: the UI asked only whether a row was in a group, then regrouped by email itself. Replaced with duplicate_group, the normalised email, which keeps the payload linear and the case/whitespace folding in one place instead of reimplemented on the client. Two new backend tests for the invite paths, one frontend test asserting the merge call keeps the verified row. The invite test fails against the un-fixed code. * fix(guests): keep guest-controlled input out of who survives a merge (#1210) Round 2 of external review on #1216. **The survivor ranking used an attacker-controlled signal.** Preferring whoever holds the most feedback looked like the obvious tiebreak and is exactly the wrong one: registration does not verify the address, so anyone who knows a guest's email can register with it, mark enough photos to out-rank the real person, and be preselected as the survivor. An admin accepting a confirmation between two rows with the same name and email would then move the victim's picks onto an identity whose token the visitor still holds. distinct_photos is guest-controlled and has no business deciding this. The ranking is now email_verified_at then created_at — both server-set. **A merge could make the survivor unrecoverable.** Rows are grouped with case and whitespace folded out, so a merge can be proposed between tina@example.com and Tina@Example.com. /guest/recover lowercases what the guest types and then matches on equality, so a survivor left holding the raw value can never be recovered by email again. The kept row's address is now canonicalised during the merge. Both write paths normalise today, so this covers rows that predate that — which are exactly the rows case-folded grouping surfaces. Two more backend tests. The residual, stated plainly: an admin can still merge two unverified rows in either order. What is gone is the tool ranking them by something a visitor controls. * fix(compose): pass GUEST_TOKEN_TTL through to the backend (#1210) The override was documented in .env.example and could never take effect: the backend service takes an explicit environment list, so a variable not named there never reaches the container. An operator following the documentation would have shortened the guest session and seen nothing change. docker-compose.production.yml uses env_file: .env and already passed it through; docker-compose.dev.yml is gitignored, so only this file needs it. * fix(guests): the admin picks the merge survivor, the tool does not (#1210) Fourth review round on the same point, and the right conclusion is that there is no correct automatic answer. Every rule tried was wrong somewhere. Most-feedback is guest-controlled — the address is never verified at registration, so anyone who knows it can register and mark photos until they out-rank the real person. Oldest-first, the replacement, is worse for the ordinary case: when a token expires the OLD row is the dead identity and the new one is the visitor's live session, so keeping the oldest deletes the identity they are actually using, and the frontend holds that deleted guest in sessionStorage without clearing it on a 401. Registration timing is visitor-controlled too. The data does not say which row is really the person. So the UI asks: merge mode gains a Keep column, the button stays disabled until a row is nominated, and only rows included in the merge can be nominated. The group is still preselected — finding the duplicates was always the point — but nothing about who survives is decided by sort order any more. This also makes the claim in the PR description true. It said the admin decides which row survives; until now the preselection quietly decided it for them. Two rewritten frontend tests: the merge is blocked until a survivor is chosen and then keeps exactly that row, and a row outside the group cannot be nominated. The test i18n mock now interpolates, so aria-labels are queryable by their rendered text. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-28 08:27:15 +02:00
Paul Nothaft	27aff7c04e	chore(main): release 3.118.0-beta.0 (#1222)	2026-08-28 06:25:14 +00:00
Paul Nothaft	22e00f80b6	feat(feedback): a third identity mode with one shared colour tag per photo (#1197) (#1208) ... * feat(feedback): a third identity mode with one shared colour tag per photo (#1197) Split out of #1178, where @boergu asked for a colour tag with no identity dimension at all: not everyone sharing a device's state, but everyone — on any device — sharing the PHOTO's state. Guest A marks it green, guest B later marks it orange, and the tag simply becomes orange. One collaboratively-agreed verdict per photo instead of per-person tallies. identity_mode gains 'shared'. The mode is scoped to the colour tag: likes, ratings, comments, favourites and reactions stay per-visitor exactly as in 'simple', because that is what was asked for and widening it would change what every other control means. Stored as an ordinary photo_feedback row under a reserved identifier rather than as a column on photos. That is what keeps the rest of the system working untouched — the per-colour tally simply has exactly one entry, so dominant_color_label, color_label_count, the admin colour filter and the XMP/CSV export that #745 reads all keep their existing shapes, and no consumer has to learn a second one. The identifier cannot be claimed: real ones are sha256 hashes or server-minted UUIDs, and the per-guest write path rejects it outright. Last write wins, inside a transaction that locks the photo row. Without the lock two guests tapping different colours in the same instant both read 'no tag', both insert, and the photo ends up carrying two shared tags — the per-guest tally this mode exists to remove. Re-sending the colour already on a photo clears it, from any guest: the same toggle every other colour path uses, and the only way to remove a tag without inventing a second control. Switching modes is non-destructive. Existing per-guest labels are left alone and simply not read while shared is on; the shared tag starts empty rather than collapsing marks nobody agreed on, and switching back restores every original exactly. An event can hold both sets, only one of which is live. The tag stays visible with show_feedback_to_guests off — it arrives through the per-viewer channel, being the photo's own state rather than someone else's opinion — while the per-colour tallies stay hidden. The colour filters answer from it for the same reason, so a gallery with sharing off cannot show colours on tiles that no filter can find. Attribution is gone by design, and the settings panel says so before an operator picks the mode. Decisions (1), (4) and (5) from the issue were settled up front, as it asked. Decision (3) turned out not to need anything: guest colour filters already read my_color_label, and the admin's my_color_labels filters photo_admin_marks (#1183), not guest identity — so nothing collapses on either side. * fix(feedback): shared mode saves on Postgres, and dormant labels stay dormant (#1197) Three findings from external review, all confirmed against source before fixing. **The mode could not be saved on Postgres at all.** Migration 078 created identity_mode with a CHECK constraint pinned to ('simple','guest'), guarded on `client === 'pg'` — so SQLite never has it and no SQLite test can see it, while the database every default production install runs rejects the new value outright. Migration 192 drops and re-adds the constraint with 'shared' included; its down() resets any event using the mode to 'simple' first, or the narrower constraint could not be restored. Verified against a real Postgres on a scratch database: the insert fails before, succeeds after, up() is re-runnable, and down() puts the old constraint back. **Dormant labels were still being read.** Switching modes is deliberately non-destructive, which leaves both sets of colour labels in the table with only one live — and every read that did not say which set it meant kept counting the other. The per-colour tallies, color_label_count, the admin grid badge, the XMP/CSV export, both admin colour filters and the guest colour filter all saw labels the mode does not show; switching back exposed the shared row as an anonymous other guest's dot. The settings panel promises these are 'kept but not shown', and that has to mean every surface, not just the badge. Scoped at the source — the two count helpers resolve the mode themselves — so the admin grid and the export are fixed without touching either. **The create form's identity mode was dropped.** CreateEventPage has always rendered the chooser and the create route never read it, so a gallery created as 'guest' came out 'simple' and had to be set again on the event afterwards. A pre-existing bug that adding a third option made worse; threaded through now, which fixes it for all three modes. Six regression tests, each verified to fail against the un-fixed code. * fix(feedback): keep every colour surface consistent across a mode change (#1197) Second review round, four findings, all confirmed in source first. **Stored counters went stale on a mode switch.** photos.color_label_count is denormalized and recomputed on feedback writes, so changing identity_mode — which changes nothing about the rows, only which of them are live — left the old mode's totals on the tiles, the admin grid and the filter summary until each photo happened to be touched again. On a finished gallery that is never. Recounted for the event when the mode actually changes, as two statements rather than a per-photo recompute: four of the five counters cannot have moved. **Duplicating an event dropped the mode**, the same shape as the create-form bug from the last round — a gallery cloned to reuse its proofing setup came back in 'simple'. **The event feedback summary counted dormant labels**, inflating total_feedback in the admin analytics and the guest /feedback-summary while every other surface hid them. **The swatch trusted its optimistic guess over the server.** In shared mode the tag belongs to the photo, so another guest can move it between this viewer's last read and their click: a viewer still showing green clicks green, the server sets green because the tag had become red meanwhile, and the optimistic 'same colour, so clear' blanked the swatch against a server that holds one. The response already says which happened, so it is used. The per-guest modes are unaffected — only the guest can move their own label, so guess and answer always agreed there. Three regression tests, each verified to fail against the un-fixed code. * fix(feedback): shared tag is not a participant, and the keyboard path reconciles too (#1197) Third review round, two findings. **feedback_count counted the shared tag as a guest.** It is COUNT(DISTINCT guest identity) across all feedback types, and the reserved identifier looked like a person: a photo with one rating and a shared tag reported two. The column is exported as rating_count (photoExportService), so merely tagging a photo inflated its rating count in the CSV and JSON exports. **The lightbox keyboard path still trusted its own guess.** The reconciliation from the last round covered clicks through PhotoColorLabels, but the proofing shortcuts call PhotoLightbox.submitColorLabel directly and set local state from a locally computed toggle. That is the path a proofing client actually uses, so it had the divergence the previous fix was for: another guest moves the tag, this viewer presses the key, the server sets a colour and the swatch blanks. Both branches now read the outcome off the response. One regression test, verified to fail against the un-fixed code. * fix(feedback): identity-mode lookup must survive a migration-time caller (#1197) updatePhotoFeedbackStats is called from migrations as well as from the request path — migration 186's duplicate-photo dedupe (#1162) recomputes the survivor's totals — and a migration runs against a half-built schema where event_feedback_settings need not exist yet. The new inner join threw there, which took the whole stats update down with it, so the reparented rows were never counted and eight assertions in the 186 suite failed. Falls back to 'simple', which is the right answer rather than merely a safe one: an install with no feedback settings table has no event in shared mode, so the non-shared scope is exactly correct. Caught by CI, not by me — I had been running affected suites rather than the full one after each review round. * fix(feedback): atomic shared-tag write, scoped feedback list, safe PG fallback (#1197) Round 4 of external review, and one of the three is about the fix I made for the CI failure two rounds ago. **The identity-mode fallback could poison a Postgres transaction.** The join was wrapped in try/catch so a migration-time caller with a half-built schema would fall back to 'simple'. On Postgres a failed statement aborts the entire transaction, so catching it and carrying on left the caller's trx poisoned and the aggregate that follows failed with 'current transaction is aborted' — defeating the very compatibility the fallback was added for. It now asks whether the table exists before issuing the join, which is safe to ask and aborts nothing. Memoised once true, since a table does not un-create itself and this sits on the feedback write path. **The shared-tag stats were recomputed after the commit.** A failure there returned 500 for a tag that had already been written, so the client reverted its swatch and the next tap on the same colour toggled the committed tag off instead of setting it. Two concurrent writers could also race their aggregate updates. Recomputed inside the transaction now, while the photo row is still locked. **The raw feedback list still carried both label sets.** Only the tallies and my_feedback had been scoped, so a dormant per-guest label was still visible to anyone reading the list — and with sharing off it came back flagged is_mine. getPhotoFeedback now filters colour labels to the active set. One test for the list; the migration suite that caught the original CI regression still passes. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-28 08:19:41 +02:00
Paul Nothaft	71eaf25d94	chore(main): release 3.117.0-beta.0 (#1220)	2026-08-28 06:15:35 +00:00
Paul Nothaft	696c69a6d0	fix(setup): put the setup token where a NAS user can find it (#1218) (#1219) ... * fix(setup): put the setup token where a NAS user can find it (#1218) The token file was never missing — it was in a subdirectory nobody opens. The all-in-one image points DATA_DIR at /data/db, so the file lands beside the database inside the single volume; someone browsing that volume from a NAS container UI sees db/, storage/, logs/, backup/ and gives up. There is no shell on those boxes to run the documented `docker exec … cat` with, and the token value is deliberately kept out of the logs, so the install looked like it had swallowed its own bootstrap credential. When DATA_ROOT names a different directory, the token is now written there too — /data/SETUP_TOKEN, the first thing visible on opening the volume. The compose stack sets no DATA_ROOT and keeps exactly one file, so nothing changes there. Each copy is written independently: the canonical one failing while the volume-root copy succeeds still leaves a readable token, and only a run where every write failed falls back to logging the value. The startup banner names every copy rather than just the first, which is what sent people into db/. Both copies are 0600 and both are removed the moment setup completes. That is what makes a second copy of a single-use bootstrap secret acceptable rather than careless — and writing the test for it turned up that the burn path had TWO independent unlinks, one in clearSetupToken and one at the end of createInitialAdmin. Only the first had been updated, so the volume-root copy survived the burn: a live-looking token that no longer works, which is worse than no token at all. Docs for the same issue are already out (PicPeak/docs#15); .env.example now names the AIO paths too. * fix(setup): enforce 0600 on a token file that already exists (#1218) External review. fs.writeFileSync's `mode` applies only when the file is created — writing over an existing inode truncates it and leaves its permissions untouched. A SETUP_TOKEN someone had copied to the volume root by hand at 0644 would keep that mode, so the first-admin bootstrap credential sat group- and world-readable on a shared NAS mount while this code claimed 0600. Unlink then create, rather than chmod after write: recreating gives a fresh inode with the right mode and no window where the credential is on disk under the wrong one. The chmod stays as a fallback for an unlink that failed for a reason other than the file being absent. Test fails against the un-fixed code. * fix(setup): drop a token copy that cannot be made private (#1218) Round 2 of external review. Asking for 0600 is not the same as getting it: a CIFS/SMB mount — which is what a NAS commonly offers — carries no Unix modes, so chmod is a silent no-op and the file keeps whatever file_mode= the mount forces, typically 0644. This feature targets exactly those hosts, so it now verifies the resulting mode instead of assuming the request took. A copy that cannot be made private is removed rather than left lying there, and it does not count as written — so an install where neither copy can be protected falls through to the existing log fallback, which reaches the operator alone. Previously a chmod that threw after a successful write left the credential on disk, and a success on the other path cleared the error, so nothing reported the exposed copy at all. Test simulates the mode-less mount with chmod as a no-op and stat reporting 0644; it fails against the un-fixed code. * fix(setup): never write the token through a foreign inode, or into the logs (#1218) Round 3 of external review, two findings, both about the credential ending up readable by someone else on exactly the shared mounts this feature targets. **The log fallback defeated the point.** When no copy can be made private, the old branch logged the token at warn — and logger.js writes warnings to combined.log under LOG_DIR, which in the all-in-one image sits on the same mount as the token file. The credential moved from a file we had just refused to leave, into another file just as readable, that outlives setup. The warning no longer carries the token; server.js already prints it on stdout when no file was written, which reaches `docker logs` without touching the shared volume. **A file that could not be deleted was written through anyway.** The pre-write unlink swallowed every error, so a 0666 SETUP_TOKEN owned by another user in a sticky or ACL-controlled directory — still writable — received the live token into its existing inode. Only ENOENT is ignored now. And when the mode check finds an exposed copy it cannot remove, that is recorded separately and reported at error level: a success on the other path clears writeError, and an exposed credential must not be silenced by an unrelated success. Two tests, both failing against the un-fixed code. * fix(setup): fail closed on an exposed token, and refuse a raced symlink (#1218) Round 4 of external review. **An exposed copy left the token valid.** A directory that permits creation and denies deletion — ACL-backed or CIFS — could keep a group/world-readable file holding a live setup token, and /setup/admin went on accepting it: anyone able to read the mount could take the first super-admin account. Reporting that was not enough. The token is now revoked when a readable copy cannot be removed, which turns what is left on disk into a dead string. Private copies are removed with it, since they hold the same value. The next boot mints a fresh one and skips the undeletable file rather than rewriting it, so this converges instead of looping on the same exposure. **The write followed a raced symlink.** On a group-writable mount another local user could drop a symlink at the path between the unlink and the write, and the default 'w' flag would follow it — putting the live token in a file they own. Now created with 'wx' (O_CREAT|O_EXCL), which neither overwrites nor follows a link; having just unlinked, anything present again is that race. The mode check uses lstat for the same reason: it must describe the file, not a link target. **A verification that threw left the file behind.** writeFileSync succeeding and lstat then failing — plausible on the network filesystems this targets — left an unverified live copy on disk, and a success on the other path cleared the error so nothing said so. Cleanup is now keyed on 'did this iteration create a file', so every post-creation failure removes it. Three tests, one new; the new one fails against the un-fixed code. Full backend suite at the known baseline. * fix(setup): report the written token path again, so the banner stays quiet (#1218) A regression I introduced one commit ago. Rewriting the write loop dropped the three lines after it that publish the result, so writtenTokenFile stayed null even on a completely successful write. server.js prints the token itself only when no file was written. With this reporting nothing, the banner took that failure branch on every fresh install and put the live super-admin setup token into stdout and `docker logs` — beside a perfectly good 0600 file. That is the exact leak this path was built to close, reopened by a refactor that touched none of the logic around it. Found by external review, not by the suite: nothing asserted the accessor, only the files on disk. Now guarded — the new test fails against the regression. * fix(setup): survive a worker race, and revoke a copy that predates this run (#1218) Round 6 of external review. **A pre-existing exposed copy was invisible to the revocation.** A restart reuses the token from the database, so an old file holding that value is a live credential. If it had become group-readable and could not be deleted, nothing tracked it — created was false, so the fail-closed path never fired and /setup/admin kept accepting what was in that file. An undeletable file at the token path is now treated as live and triggers the same revocation. **A losing worker printed the token.** The shipped PM2 cluster config runs several workers against one DATA_DIR. Both pass the unlink, one wins the exclusive create, and the loser's wx write threw EEXIST — so it recorded nothing and its banner printed the live token into its own log while a perfectly good 0600 file already existed. EEXIST now checks the file: private, regular, and holding the same token counts as this loop's work already done. **A write that created the file and then threw left it behind.** ENOSPC, a short write, a delayed close on a network mount — writeFileSync can populate the inode before failing, and cleanup keyed on the call returning skipped it. Keyed on the write being attempted now, with an existence check. Two tests, both failing against the un-fixed code. Full backend suite at the known baseline (2342 passing). * refactor(setup): drop the volume-root token copy, keep the hardening (#1218) The second copy was for discoverability: DATA_DIR points into /data/db on the all-in-one image, and a NAS user browsing the volume does not open a folder called db. Six review rounds later it had earned a second inode to race, to verify, to clean up and to revoke — a symlink guard, an exclusive create, an lstat check, cluster-race handling and fail-closed revocation, nearly all of it load-bearing only because there were two files instead of one. That is a lot of attack surface for a convenience the documentation covers better. PicPeak/docs#15 now points NAS users at ADMIN_PASSWORD, which creates the admin on first boot and needs no file at all, and names the db/ subdirectory for anyone who does want the token. Neither needs a second copy. So: one file in DATA_DIR again, as before. Everything the review turned up stays, because none of it was about the second copy — the token is created with O_CREAT|O_EXCL so a raced symlink cannot capture it, its mode is verified with lstat rather than assumed, a copy that cannot be made private is removed, one that cannot be removed revokes the token instead of being logged about, a partial write is cleaned up, a concurrent worker's good file is accepted rather than triggering the log fallback, and the token never reaches the log files. setupTokenFilePaths and writtenSetupTokenFiles are gone with their tests; the hardening tests remain and still fail against unfixed code. * fix(setup): publish the token atomically instead of racing over one inode (#1218) Round 7 of external review found a race in the exclusive-create approach: two PM2 workers reaching the write together, the loser sees the winner's file after the inode exists but before its content lands, judges it wrong, and deletes it — after which the winner's own verification fails too, both report nothing written, and both print the live token into their logs. Rather than teach the loser to wait, the shared inode is gone. The token is written to a per-process temporary file, verified there, and published with rename(2). That is atomic: the file never appears at the published path with the wrong mode or half its content, a symlink sitting at that path is replaced rather than followed, and concurrent workers simply publish the same value one after another. The unlink-then-create dance, the EEXIST handling and the cross-worker deletion all disappear with it. Verifying the mode BEFORE the rename is the stronger order too: a credential that cannot be made private on a mode-less mount now never reaches the published path at all, instead of being written and then cleaned up. If publishing fails and something is still sitting at the token path, it is treated as a live credential we could not replace, and the token is revoked — unchanged in intent from the previous round, simpler in mechanism. * fix(setup): drop a dead assignment and an unused import (#1218) Both flagged by the code-quality review on #1219. `createdTmp = false` after rename(2) is never read — rename consumes the temp file, so the catch has nothing left to clean up either way. `os` was never used in the test. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-28 08:08:54 +02:00
Paul Nothaft	a490b64954	fix(admin): the "Uncategorized" photo filter returns every photo (#1211) (#1214) ... The dropdown offered the filter and it never worked. It rendered as `value="0"`, and adminPhotos.js skips '0' outright: if (category_id !== undefined && category_id !== '' && category_id !== '0') { so no category condition was applied and the whole event came back. Four lines below that guard sits the branch that does the work, keyed on the literal 'uncategorized' — which nothing was sending. The two ends have never agreed on the wire value, and neither is wrong on its own. It fails silently, which is why it went unnoticed: a full list reads as 'the filter found nothing to narrow' rather than 'the filter did not run'. Send what the backend already understands rather than teaching it a second spelling. The onChange passes non-numeric values through unchanged, so the string arrives intact. Reported in #1209 by someone re-categorising a few thousand photos imported without a category — the filter is the first step of filter, Select All, bulk assign, so its failure takes the whole path with it. Tests both ends of the contract, since the bug was the pairing rather than either half: the frontend emits 'uncategorized', and the endpoint answers it with only the null-category rows. The backend test also pins that 0 means no filter, so a future change there has to be a decision rather than an accident. Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-28 08:05:32 +02:00
Paul Nothaft	0a36ca6056	feat(gallery): folders that contain photos instead of filtering them (#1160) (#1161) ... * feat(gallery): folders that contain photos instead of filtering them (#1160) A category has always been a filter: its photos stay in the root grid and picking the category narrows that grid. D#1086 asked for the opposite — put the selects in a bucket and get them OUT of the main grid, so the client sees the 40 finals and clicks through for the other 200. `photo_categories.is_folder` makes that a per-category choice. One column is enough because the neighbouring features already built the substrate: hero_photo_id (#163) is the folder cover, allow_downloads (#640) is per-folder download rules, display_order + event_category_order (#782) is folder ordering, and photos.category_id being single-valued is already folder semantics. Deliberately no parent_id. "Root -> Selects folder" is depth one, i.e. plain containment; folders-inside-folders waits until someone asks. Containment lands in the one useMemo where the category filter was already applied, and the tiles render above the grid rather than inside a layout, so all eight gallery layouts inherit folders without eight implementations. Scope drives the counts too, so root reports 40 photos and not 240. `?folder=<slug>` carries the open folder, preserving token and admin_preview, so a folder is linkable and Back walks out of it instead of leaving the gallery. Defaults to false, so every existing gallery keeps filtering exactly as before. Folders are organisational, not access control: a foldered photo is served by the same per-photo auth as any other. A test pins that, so nobody later mistakes containment for a security boundary. * feat(gallery): download a folder on its own, and label folders when moving photos (#1160) Downloads now cover both halves of the requirement: - the gallery-wide "download all" keeps zipping every photo including the foldered ones (verified: 62 files), so a folder never quietly removes photos from the client's one-click download; - inside a folder there is a "Download folder (n)" button that zips only that folder, once (verified: 20 files). It reuses /download-selected, so there is no new endpoint and no second zip-building path. The button honours the per-category opt-out (#640) both ways: a folder with allow_downloads = false renders no button, and individual photos that opted out are excluded from the id list rather than silently 403-ing mid-zip. Moving photos into a folder already worked — a folder IS a category, so the existing bulk "move to category" flow does it. What was missing is that a folder and a filter category looked identical in that dropdown while having very different consequences, so folder options now read "<name> (folder — hidden from the main grid)". Threading is_folder through to the dialog needed the admin category prop types widened; the data was already on the wire. * fix(gallery): folders were unreachable in the full-bleed layouts (#1160) Containment comes from `filteredPhotos`, which BOTH layout branches use, but the tiles were only rendered in one. On a Premium or Story gallery the foldered photos therefore disappeared from the grid with no tile to click — moving 200 selects into a folder effectively deleted them from the client's view. The folder nav is now built once and rendered by both branches, so a branch can't hide photos without also offering the way in. Those two layouts are edge-to-edge by design, and a block of cover cards above the hero wrecks the opening they exist for, so they get a compact chip row (`Folders [icon Selects 20]`) instead. It only renders when the gallery actually has folders, leaving every existing full-bleed gallery byte-identical. Also scopes the people strip to the photos on screen. `face_count` comes from /people and spans the whole event, which contradicted the grid in two ways: inside a folder a face read "12 photos" but filtered down to the handful in that folder, and at root a person whose photos ALL lived in a folder showed up and filtered to nothing — a dead chip. Recounted from `photo.person_ids`, which is already what the filter itself uses, and zero-count people are dropped. No backend change; the ids were on the wire already. Verified in the running app: Premium renders the chip and navigates; the lightbox counter inside a folder reads "1 / 20", not 1 / 62; the people strip inside the folder drops from 12/11/4 to the one face actually present. * fix(gallery): folder edge cases found in external review (#1160) Seven issues, all verified against the code before fixing. Unreachable folders (the serious one). `adminCategories` derives a slug with `[^\w\s-]` stripping and `\w` is ASCII-only, so a valid name in a non-Latin script slugs to the empty string — `Избранное` and `日本語` both do. Keying the URL on the slug meant such a folder wrote no param and resolved to nothing: its photos left the root grid with no way back to them. This repo ships ru and sl locales, so that is a reachable state, not a hypothetical. Folders are now keyed by `folderKey()` — slug when there is one, id otherwise. Stale selection across a scope change. The grid clears its selection when `categoryId` changes, which is already null at root, so a selection made outside a folder survived into it and the toolbar would offer to download (or a client to hide) photos no longer on screen. Cleared on both explicit navigation and popstate. Dead category chips inside a folder. The filter branch ignores `selectedCategoryId` while a folder is open, so the chips did nothing when clicked. They are no longer offered there. "No photos found" beside folder tiles. A gallery whose photos all live in folders rendered the tiles and then the grid's empty state directly under them, claiming the gallery was empty while pointing at its contents. Counts that contradicted the grid. The filter bar and both people surfaces were still counting over every event photo, so a chip could advertise a total the scoped grid would never produce. All now count over `scopedPhotos`. `!!` on a validated boolean. express-validator's isBoolean() accepts the STRINGS "false" and "0", and `!!'false'` is true — a form-encoded caller asking for a filter would have silently got a folder. Uses the existing parseBooleanInput. Duplicate-event dropped folder-ness. The category clone selected only name, slug and is_global, so every folder in a duplicated gallery came back as a filter. * fix(gallery): folder scoping gaps from external review round 2 (#1160) Cache mutation, introduced by this branch. `photosInScope` returned the caller's own array on the no-folders fast path, and `filteredPhotos` sorts in place — so every gallery WITHOUT folders was reordering the React Query cache for every other consumer of `data.photos`. The pre-branch code cloned; now it always does. Colliding folder keys. UNIQUE is (slug, event_id), so a global folder and an event folder can share a slug, and the gallery merges both scopes. Keying on the slug alone meant the second folder resolved to the first and its photos could not be opened. The id is now always part of the key. "Download folder" downloaded a subset. Search, feedback, media and people filters stay active when entering a folder, and the ids came from `filteredPhotos` — so the button promised the folder and delivered whatever the filter had left, or vanished when it matched nothing. Built from `scopedPhotos`. Folder-only root misdetected. `rootIsFoldersOnly` tested `filteredPhotos`, so a search matching none of the loose root photos looked folder-only and swallowed the no-results message. Tests the unfiltered scope instead. Empty state in the full-bleed layouts. The Premium/Story branch was missing the folder-only guard the standard branch got, so a folder-only gallery printed "no photos found" under its own folder chips. Filter metadata still event-wide. `availableMediaTypes` and `colorLabelCounts` counted over every photo, so the sidebar could offer a Video or colour chip for something that only exists in another scope — always filtering to nothing. Both derive from `scopedPhotos`, which moved above them for that reason. * fix(gallery): honest folder downloads and scoped totals (#1160) Silent truncation. /download-selected slices the id list to 500 server-side (gallery.js:1776), so a folder larger than that delivered a truncated archive under a button promising the whole thing. The limit is now mirrored client-side: the request carries only what the server will honour and the label says "Download first 500 of 620" instead of claiming the folder. Gallery shell was being unmounted. Suppressing the folder-only empty state by skipping PhotoGridWithLayouts took the hero, event title, logout and download controls with it in the full-bleed layouts, since those render from inside that component — a folder-only Premium gallery collapsed to a bare chip row. Replaced with a suppressEmptyState prop so only the message goes. Two more counts that could contradict the grid: the sidebar's total and the people match-count denominator ("42 of 62" at a root that holds 42). Both scoped. The client-access visible/total stat is deliberately left event-wide — that one is a photographer-facing statistic about the gallery, not a filter affordance. Stale admin cache. EventDetailsPage caches the same category rows under 'admin-event-categories' and hands them to the Photos tab's move dialog, so toggling a folder left that dialog labelling it a plain category until remount. Both keys are invalidated now. Not changed, after challenging the review: select-all in the full-bleed layouts stays scoped to the displayed photos. Wiring it to the full event would select photos that are not on screen, contradicting containment and reviving the stale selection bug. The reviewer withdrew the finding on that basis. The residual UX gap — no one-click "everything" in Premium/Story once folders exist — is real and noted on the PR. * feat(gallery): one-click download-everything in the full-bleed layouts (#1160) Premium and Story have no header download button — their only gallery-wide download is select-all followed by download-selected, and select-all is correctly scoped to what is on screen. Once folders exist that left no single way to get the whole gallery. The folder strip now carries an event-wide "Download all photos" that hits /download-all (which has always included foldered photos), shown at the root only, since inside a folder the breadcrumb already offers that folder's download. Also lands the capped folder label that was written but never actually applied in the previous commit — the edit silently didn't match, so a 510-photo folder still advertised "Download folder (510)" while the request was capped to 500. Caught by building a real 510-photo folder rather than trusting the reasoning: it now reads "Download first 500 of 510". A unit test pins the client constant to the backend's cap so the two can't drift apart unnoticed. * fix(gallery): remount layouts on folder change, and stop scoped counts leaking into event-wide controls (#1160) Carousel crash. Layout state is only meaningful for the photo set it was built against, but the layout instance was reused across a folder change. In carousel mode an index valid at root (31 of 42) indexes past the end of a smaller folder, and CarouselGalleryLayout does `photos[currentIndex]` unguarded. The grid is now keyed by the open folder, so a scope change remounts: verified live, 31/42 at root becomes 1/20 on entering the folder instead of dereferencing undefined. The key also avoids driving one instance between the empty and non-empty render paths, which matters because that component's `photos.length === 0` early return sits ABOVE four useState calls — a pre-existing conditional-hook hazard this feature would otherwise have made reachable. Nested empty state. suppressEmptyState only silenced PhotoGridWithLayouts' own early return; the Premium and Story layouts have their own noPhotosFound return, so a folder-only root still printed "no photos" under the tiles proving otherwise. The flag is forwarded to them. Download All was labelled from the wrong number. The sidebar's total is now the folder scope (correct for the category list), but the same value labelled and disabled Download All — which fetches the event-wide archive. On a folder-only root that showed 0 and refused a valid download. Split into a separate downloadAllTotal. Feedback chip counts. likeCount, favoriteCount and ratedCount still counted over every event photo while clicking them filters the scope, so a chip could promise matches from another folder and deliver none. * fix(gallery): premium crash, story Download All, and empty-mount hazard (#1160) ReferenceError blanking the Premium gallery — my own bug from the previous commit. The suppressEmptyState prop landed on the nested PhotoCard instead of GalleryPremiumLayout (both destructure `allowDownloads = true`, and the patch hit the first one), so the layout's guard referenced an identifier that was not in its scope. A folder-only Premium root threw instead of rendering. Now declared and destructured on the layout, and exercised: 62 photos all foldered renders the tile, the hero and the download button with no message and no throw. Story's footer "Download All Photos" built its id list from the `photos` prop, which is now the folder scope — so it silently omitted every foldered photo while still calling itself Download All. Layouts now receive an event-wide downloadAllIds and prefer it. Premium's equivalent control is a select-all, not a download, and stays scoped by the same reasoning as before. Empty-array mounts. Suppressing the empty state meant the layout got mounted with photos=[], and CarouselGalleryLayout returns before four of its useState calls — driving one instance between empty and non-empty changes its hook count and React throws. Only the full-bleed layouts, which own the hero and logout chrome, are now mounted empty; every other layout renders nothing instead. * fix(gallery): keep the shell and drop dead controls on folder-only roots (#1160) Skipping the empty layout took the hero and welcome message with it. The early return sat above both, so a gallery whose photos all live in folders lost its configured hero and welcome copy at the root and only regained them after opening a folder. Only the layout child is skipped now; the surrounding shell renders as it always did. The filter bar was gated on the event-wide photo count, so a folder-only root still rendered search, sort and the feedback chips with nothing in scope for them to act on — the same empty filter row discussion #317 asked us to remove. Gated on the current scope. Story's download toast counted `photos` while the request now carries the event-wide id list, so it could announce "Downloading 0 photos" and then fetch the whole gallery. Counts the ids it actually sends. * fix(gallery): clear the person filter on scope change, and fix two folder-only shell details (#1160) A person selected in one scope can have no photos in the next. peopleInScope drops them from the strip, so the filter stayed active with nothing left to clear it — and the full-bleed layouts have no people UI at all, leaving a guest staring at an empty grid with a reload as the only way out. Cleared on both folder navigation and popstate, alongside the category selection and the photo selection already reset there. Story's hero announced "0 Photos" on a folder-only root, since it derives that stat from the scope it renders and the scope is empty by definition there. Falls back to the event-wide count. Premium's integrated Download All is a select-all over the current scope, so on a folder-only root it was a visible control that did nothing when clicked. It is hidden while the scope is empty rather than left dead. * fix(gallery): uncapped Story download, protected folder covers, scoped people order (#1160) The event-wide id list I added for Story's "Download All Photos" made it worse, not better: /download-selected caps at 500 ids server-side, so a gallery larger than that silently shipped a partial archive under a button promising all of it. Replaced with an onDownloadEverything callback that runs the whole-gallery /download-all path, which has no cap. eventPhotoCount now carries the number Story needs for its hero stat, so no id list crosses the boundary at all. Folder covers bypassed image protection. A cover is a real gallery photo, but it was rendered through AuthenticatedImage's defaults while every photo tile passes the gallery's protection settings — so on a gallery configured for canvas rendering or maximum protection, each cover was an ordinary blob-backed <img>. The tiles now receive and apply the same props as the grid. People kept /people's event-wide ordering after their counts were rescoped, so a folder's most-photographed person could sort behind someone with a single match — and PeopleStrip only shows the first twelve inline. Sorted by the recomputed count, with a test. * fix(gallery): folder covers honour maximum protection (#1160) Maximum protection implies canvas rendering even when the independent use_canvas_rendering toggle is off, which is its default — every other gallery image path spells that out as `useCanvasRendering || protectionLevel === 'maximum'` (PhotoGrid, PhotoLightbox, HeroHeader, JustifiedGalleryLayout). The folder cover forwarded the raw toggle, so on a maximum-protection gallery with the toggle untouched the cover fell back to a blob-backed <img>. Matches the convention now. * fix(gallery): don't let download-everything bypass a category opt-out, and keep folder links alive across renames (#1160) The whole-gallery route serves a prebuilt zip containing EVERY event photo with no per-category filter — gallery.js says so itself, next to bumpEventDownloadCounts, as a known pre-existing gap. Wiring Story's footer to that route therefore converted a path that DID enforce the #640 opt-out into one that doesn't, and because the callback was supplied unconditionally it affected Story galleries with no folders at all. The same reasoning applies to the download-everything button this branch added to the full-bleed folder strip: it routes there too, so on a gallery with a restricted category it would have handed over exactly the photos the opt-out withholds. Both are now withheld whenever any category opts out; those galleries keep the per-folder download, which enforces it. Verified both ways — the control disappears with a restricted category present and returns once the restriction is lifted. Folder links also survived a rename badly: the key embeds the slug for readability, and renaming a category rewrites that slug, so a URL already sent to a client stopped matching and silently opened the gallery root. Resolution now keys on the trailing category id, which does not move. * fix(gallery): make folder navigation clickable in the Story layout (#1160) Story renders `.story-nav` as `position: fixed` across the top of the viewport at z-index 50, and the folder strip sits in exactly that band — so the nav swallowed every click on the chips and the breadcrumb. A Story gallery whose photos all live in folders had no way to reach them at all. Confirmed with elementFromPoint at the chip's centre returning NAV.story-nav; the strip now carries its own stacking context above it and the same probe returns the chip. Story's footer download could also be offered with nothing to send: on a folder-only root of a gallery that has a category download opt-out, the parent deliberately withholds the whole-gallery callback and the scope is empty, so the button would have posted an empty id list and taken a 400. It is only rendered when one of the two actually exists. * fix(gallery): stop the Story folder strip from blocking the layout's own nav (#1160) The previous commit raised the whole folder strip above `.story-nav` so the chips could be clicked, and thereby traded the bug for its mirror image: the strip is mostly empty space, so as a solid z-60 container it swallowed the clicks for Story's own search, favourites and logout sitting underneath. The container no longer takes hits at all; only the chips, breadcrumb and download button opt back in. The download button also loses its ml-auto, since being pushed to the right put it physically on top of the nav's controls rather than merely above them in stacking order. Verified by hit-testing all three at once — folder chip, download button, and Story's nav control each resolve to themselves under elementFromPoint, so none is covering another. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-28 08:03:49 +02:00
Paul Nothaft	56cf947735	chore(main): release 3.116.1-beta.0 (#1206)	2026-08-27 06:50:11 +00:00
Paul Nothaft	cec8eff70c	fix(images): fence the capture-date backfill on the file it read (#1201) (#1204) ... The capture-date backfill committed its result keyed on the row id alone. It snapshots every candidate up front, then walks them one at a time reading originals off S3 or a NAS mount — a pass that can run for many minutes. replacePhoto, reachable from the replace_by_name upload path, swaps a NEW file under an existing row and rewrites path/filename. A replacement landing inside that window carries no date of its own, so captured_at was still NULL, the whereNull guard passed, and the previous file's EXIF date was written onto the new photo. Silent: nothing errored, the run reported it as a success, and the gallery just sorted that photo to the wrong place. Fenced on path and filename as well as the id — the same fence #1199 put on the orientation backfill for the same reason — so a replaced row matches zero rows and is skipped. The candidate query already selects both columns, so no query change. Knex renders a null value in the object form as `is null` on both the pg and sqlite3 clients, so a row with a NULL path still matches itself. Those skipped candidates are now counted rather than dropped. replacePhoto is not the only writer of path/filename — eventRenameService rewrites both on an event rename, which is not a content change — and another writer filling captured_at first lands in the same place. Without a counter they fell out of the run's arithmetic entirely: success + noExif + failed no longer added up to the count the operator was shown when they started the job, on the card as well as in the log. The card shows the count only when it is non-zero, the same shape the orientation job uses for staleTiers. The wording states what is known — changed by something else, not updated — rather than promising a retry: for the already-dated case there is nothing to retry, and the Missing Capture Date figure above is what says whether work is left. Locale coverage matches the staleTiers key (en, de, fr, sl), with the defaultValue carrying the rest. Regression test: a replacement landing mid-run leaves captured_at NULL and is not counted as updated. Verified to fail against the unfenced code.	2026-08-27 08:44:19 +02:00
Paul Nothaft	fb7af502ec	chore(main): release 3.116.0-beta.0 (#1203)	2026-08-26 19:20:50 +00:00
Paul Nothaft	d3e9a7cf0d	feat(auth): make the admin "Remember me" checkbox actually do something (#1186) (#1195) ... The checkbox had no `checked`, no `onChange`, and no place in the login request; `rememberMe` existed only as an i18n label. On the backend establishAdminSession hardcoded `expiresIn: '24h'` and the cookie always got DEFAULT_MAX_AGE_MS, so there was nothing to receive it anyway. Wired end to end: state on the page, `remember_me` in the login body, and a 30-day JWT plus a matching 30-day cookie when it is set. Opt-in on purpose. An absent or malformed value means "no", so a client that never sends it keeps exactly the 24h session it always had, and a stolen cookie is still worth a day by default. The JWT and the cookie take their lifetime from the same flag. If they can disagree the session either dies early (long cookie, short token) or outlives what the user consented to, so the tests assert them against each other. Review found the feature was non-functional as written, which is the important part: sessionTimeoutMiddleware and isSessionExpired enforce security_session_timeout_minutes — 60 minutes by default — against a session's idle time regardless of how long its token lives, so a remembered admin was logged out within the hour with a 30-day token sitting unused. rememberMe now travels in the JWT payload and both checks exempt a remembered session from the IDLE timeout. Not from expiry: the token still dies on its own 30-day exp, and revocation, deactivation and password-change invalidation are untouched. Also: /api/admin/auth/change-password reissued a hardcoded 24h token without the flag, so a remembered admin dropped back to 24h the moment they changed their password — which is mandatory for new and reset accounts. It now inherits the choice from the session it replaces, carried on req.admin.rememberMe. Through MFA the choice rides inside the signed mfa_pending token rather than being resent, so the second leg cannot ask for longer than the first agreed to. The tests drive POST /api/auth/admin/login and read the real Set-Cookie and token rather than minting a local clone of the ternary they are meant to be checking, boot one database per file before anything reads it, and generate their credential per run so no literal that looks like a password lands in the repository. No visual change — the checkbox was uncontrolled, so it already toggled on click; it just did nothing. Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-26 21:13:23 +02:00
Paul Nothaft	95e7301909	chore(main): release 3.115.4-beta.0 (#1200)	2026-08-26 19:08:03 +00:00
Paul Nothaft	edef4d7365	fix(images): backfill orientation for libraries that predate the fix (#1199) ... * fix(images): backfill orientation for libraries that predate the fix (#1198) #1194 corrected the generators and every ingest path, but did nothing for photos already in the database. Those rows end up worse than untouched ones: before the fix a rotated photo was CONSISTENTLY wrong — a sideways image in a tile shaped to match — and afterwards the regenerated thumbnail is correct while photos.width/height still describe the raw sensor order, so masonry and justified size a portrait photo with a landscape ratio. The dimension repair cannot reach them: it only selects rows with a NULL dimension, and an affected row has both, just transposed. Its own job rather than a mode of that one. They look alike but are not the same operation: the repair FILLS missing values and touches nothing else, while this RECOMPUTES and invalidates the derived data generated against the old orientation. Sharing a lease would also mean one blocks the other. A first attempt at this was reverted from #1194 after review found five problems. All five are addressed here: - Originals are read through resolvePhotoStorageKey + withLocalCopy + withProcessableImage, so the job works on S3 installs and on RAW/DNG. The dimension repair's direct fs read does neither, which stops being an edge case in a job that walks the whole library. - The canonical preview is cleared BEFORE faces are requeued. ensurePreviewImage returns a cached preview whenever it is still a valid image, and a pre-fix unrotated one is perfectly valid — so requeueing alone made the rescan read unrotated pixels and scale those boxes by the corrected dimensions, which is worse than leaving the data alone. - Invalidation keys off the EXIF transform, not a dimension delta. Orientations 2, 3 and 4 move every pixel while leaving width and height unchanged, as does 5-8 on a square image; a delta check skips exactly those rows. - Archived events are excluded — archiving deletes the originals and keeps the rows, so every one of them would fail its read. - The dimension write and the invalidation share a transaction. Split, a failure between them leaves stale face data that no retry can fix, because the retry computes "already correct". Tier deletion stays outside the transaction on purpose: it touches storage, and a failed object delete must not roll back a correct database write. A leftover tier regenerates on next read; a rolled-back write is silent corruption. * fix(images): invalidate every stale rendition, fence the writes, and give the job a button (#1198) Three things from review, one of which mattered a lot. The invalidation was too narrow. Clearing only preview_path fixed the face data and left the gallery worse off: ensureThumbnail and ensureHeroImage return their cached file whenever it is merely VALID, and a pre-fix sideways thumbnail is perfectly valid — so a corrected row rendered the old sideways image inside a newly-corrected portrait tile. All three canonical renditions are cleared now, their stored objects deleted, and both responsive tier sets with them. The responsive tiers also needed handling rather than a hopeful catch. Their helpers swallow delete errors, and ensurePreviewImageAtWidth treats storage.stat(key) as a cache hit — so a tier that survived deletion keeps serving unrotated forever and never regenerates. The keys are re-checked after deletion and survivors are counted into the result, so a run that could not clear them does not report itself as clean. Writes are fenced on the identity that was measured, not just the id. replacePhoto swaps a new file under an existing row and rewrites path/filename, and it IS reachable — from the replace_by_name upload path in adminPhotos.js. A replacement landing while this job read the old original would otherwise have had the previous file's dimensions written over it and its fresh renditions cleared. And the job had no way to start it: the endpoint existed with no caller, so an upgrade would have left every affected library untouched unless an operator found the API themselves. It gets a Status card like its two neighbours, with strings in en/de/fr/sl. No backlog counter, because unlike the other two it cannot know how many rows need it without doing the work. * fix(images): make the backfill idempotent, and stop it lying about what it did (#1198) Six things from review round 2. The job was not idempotent, and the way it failed was expensive. Its trigger is the EXIF tag on the ORIGINAL, which correcting a photo never changes — so every re-run threw away the renditions it had just regenerated and requeued every completed face scan. On a face-enabled install, running it twice meant re-detecting the whole library for nothing. Migration 191 adds photos.orientation_checked_at, written in the same transaction as the work it records, with `force` as the escape hatch for an interrupted run. The candidate query selected preview_path but not thumbnail_path or hero_path, which the deletion loop reads — so those two pointers were cleared in the database while the objects stayed in storage, still reachable through previously issued URLs. watermark_path was missed entirely. gallery.js serves it ahead of the original when branding watermarking is on, which makes it the most visible rendition of the lot. (Its generator needed rotating too — that went into #1185, where the other three live.) storage.stat() RESOLVES with null for a missing key rather than rejecting, so counting "the promise settled" marked every deleted — and every never-created — tier as a survivor. A perfectly clean run told the operator to re-run. Now a null means gone, and a rejection counts as stuck, since a storage error is not proof the object went away. Face data is invalidated whenever the stored dimensions change, not only when the change came from rotation: boxes are scaled by photo.width at read time, so any dimension change strands them. And `corrected` now comes from the affected-row count. If the fence rejected the write because the file was replaced mid-run, the photo was not corrected and the run must not claim it was. * fix(images): stop the backfill doing unnecessary work, and make its retry advice true (#1198) Round 3, four points, all narrower than the last two rounds. It re-processed photos that were already correct. A 5-8 rotation changes the dimensions, so a tagged photo whose stored dimensions are ALREADY oriented must have been ingested after #1185 — its renditions are fine and clearing them deletes valid files and rescans a completed face detection for nothing. Those are now skipped and simply marked. Orientations 2, 3 and 4 (and 5-8 on a square image) leave the dimensions identical either way, so they carry no such evidence and are still done once. The retry advice was impossible to follow. When a responsive tier could not be deleted the row was still marked, so the ordinary re-run the UI recommends found nothing and the stale tier kept serving unrotated forever. The marker is withheld when a tier survives, which is what makes that message honest. Storage cleanup now only runs when a fenced write actually landed. If the file was replaced mid-run every update matched zero rows, but the deletion went ahead anyway and could destroy renditions belonging to the REPLACEMENT — watermarks especially, which are keyed by photo id and alias straight onto the new file. And the full-photo ETag includes the backfill's timestamp. It was built from the ORIGINAL's mtime plus the watermark settings hash, neither of which this job touches — so a guest holding a pre-fix ETag would go on getting 304 and their cached sideways image no matter how many times the backfill succeeded. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-26 21:01:51 +02:00
Paul Nothaft	c18f54ede0	fix(images): respect EXIF orientation in thumbnails, heroes and previews (#1194) ... * fix(images): respect EXIF orientation in thumbnails, heroes and previews (#1185) generateThumbnail, generateHeroImage and generatePreviewImage went straight from sharp(imagePath) to .resize(), so a photo whose Orientation tag is not 1 — routine for portrait shots on bodies that tag rather than rotate the sensor data — was resized from the raw frame and came out sideways. The same pipelines then call .withMetadata(false), stripping the tag from the output, so nothing downstream could correct it either. The download path already had this right: resizeToBox calls probe.rotate() for stills, which is why the same photo looked correct on download and rotated in the gallery. All three generators now do the same, guarded to stills for the reason resizeToBox already documents — .rotate() flattens a multi-frame source. The reporter also spotted the half that compounds it: photos.width/height were stored from sharp's metadata, which reports pixels as STORED, not as displayed. For orientation 5-8 those are swapped, so a portrait photo landed in the database as landscape and masonry/justified sized its tile with the wrong aspect ratio on top of the image being unrotated. A shared orientedDimensions() helper now does that conversion at all four capture sites — managed upload, background processing, external import and the dimension repair — so the stored numbers describe the rotated result the generators now produce. Existing rows keep their pre-rotation dimensions until the photo is reprocessed; the images themselves correct on the next thumbnail/preview regeneration. Tests fail on the unfixed generators — verified by reverting the rotate calls and the swap, which fails 4 of the 7. * fix(images): orient dimensions on every ingest path, and stop guarding rotate where it protects nothing (#1185) Review found the first cut covered four of eight dimension-capture sites. The filesystem watcher, the S3 auto-importer, the v1 upload API and replace-by-name all still persisted raw metadata.width/height, so an orientation 5-8 photo arriving that way got a correctly rotated thumbnail and a database row describing it as landscape — the same aspect-ratio mismatch this PR set out to remove, just on the paths I had not grepped. (I searched for `metadata.width` and the v1 route aliases it to `meta`.) The animated guard was also wrong in two of the three generators. generateThumbnail and generateHeroImage never pass `animated: true`, so they already flatten a multi-frame source to its first frame — skipping .rotate() there protected an animation that was being discarded anyway, while leaving the output in raw orientation against swapped stored dimensions. Both now rotate unconditionally. generatePreviewImage keeps the guard, because it genuinely does open animated sources as animated and .rotate() would flatten them. That leaves one corner unsolved rather than papered over: a multi-frame source that also carries an orientation tag keeps its raw orientation in the preview while the thumbnail and stored dimensions describe the rotated one. GIF has no EXIF and animated WebP effectively never sets it, so it is a real gap but not a common one, and closing it means rotating frame by frame rather than quietly dropping the animation. Documented at the guard. * fix(images): add a recompute mode so existing libraries get corrected too (#1185) The orientation fix only helped new photos. A row affected by the bug has BOTH dimensions stored — just in the raw order — so the repair job's NULL filter could never reach exactly the rows that needed it. Worse, once their thumbnails regenerated rotated, those rows went from consistently-wrong (sideways image in a matching tile) to inconsistent: correct image, wrong-shaped tile. `recompute` widens the candidate set to every image row. Opt-in, because it re-reads every original. It also has to deal with the consequence for faces. Detection runs against the preview and stores boxes in ORIGINAL pixel space, scaled by `photo.width / previewMeta.width` (faceProcessor.js:220-224) — so a photo whose stored dimensions change has face data recorded against a coordinate system that no longer exists, and the overlays crop the wrong region. Photos whose dimensions actually change are requeued for scanning; ones that were already correct are not, or a routine repair would rescan the whole library. Rows with face_status NULL are left alone so installs that never enabled the feature don't start scanning because of a dimension repair. Writing the test for that last rule caught a real bug in it: the candidate query never selected photos.width/height, so `photo.width` was undefined and every row compared as changed. Both columns are selected now. * Revert "fix(images): add a recompute mode so existing libraries get corrected too (#1185)" This reverts commit cb771d08. Review round 3 found five problems, all of them in this addition rather than in the orientation fix itself, and one of them an own-goal: requeueing face scanning makes processPhotoFaces call ensurePreviewImage, which returns the CACHED pre-fix preview when it is still a valid image — so the rescan reads unrotated pixels and scales those boxes by the newly corrected dimensions. That is worse than leaving the data alone. The rest need work this PR should not be carrying: the dimension repair reads originals through resolvePhotoFilePath and plain sharp, so it does nothing on an S3 install and rejects RAW/DNG; recompute pulls archived rows whose originals were deleted on archive; orientation 2, 3 and 4 change the pixels without changing width or height, so a dimension-delta test never notices them; and the dimension write and the face invalidation are not atomic, so a failure between them leaves a row that no retry will ever requeue. Split out so it can be designed and reviewed on its own. The orientation fix — .rotate() in the three generators and orientedDimensions() at all eight ingest sites — is unaffected and stays. * fix(images): the watermarked rendition needs orienting too (#1185) A fourth generator with the same bug, found while reviewing the backfill that builds on this. watermarkService composites and re-encodes through its own sharp pipeline with no .rotate(), and gallery.js serves photos.watermark_path ahead of the original when branding watermarking is on — so on a watermarked gallery the sideways image is precisely what a guest sees. Two details this needed beyond the .rotate() itself: metadata() is read from a separate, unrotated handle. .rotate() does not change what metadata() reports — a 400x200 source tagged orientation 6 still reads 400x200 — and every use of those numbers here is positioning: watermark scale, font size, composite extent. They have to be the DISPLAYED dimensions or the mark is placed against the wrong axis, so they go through orientedDimensions. The composite offsets are floored. getPositionCoordinates derives from the SVG's estimated text extent and returns fractional pixels; sharp rejects a non-integer offset and applyWatermark catches its own error and returns the image unwatermarked. Landing on a whole pixel was luck, and changing the dimensions it is computed from ran out of it — the test surfaced a real "Expected integer for left but received 92.8". --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-26 20:55:54 +02:00
Paul Nothaft	2d98f5fa9f	chore(main): release 3.115.3-beta.0 (#1192)	2026-08-26 07:02:21 +00:00
Paul Nothaft	3991dc3ccb	fix(admin): gate the dimension repair as system maintenance (#1182) ... * fix(admin): gate the dimension repair as system maintenance (#1181) The endpoint's candidate query is unscoped, so it walks every event in the install, reads every original off S3 or the NAS mount, and rewrites their metadata. It required only photos.edit, which the built-in team_photographer preset holds (175_granular_permissions_and_presets.js:106) — a role that exists for a contributing second shooter, not for someone who should be able to start a whole-library scan or touch another owner's events. Now system.manage, whose own description is "run system maintenance actions", with the status endpoint on system.view to match. Nobody who should have it loses it: super_admin is granted every permission, solo_photographer is 'ALL', and migration 175 already projects every settings.edit holder forward onto system.manage on upgrade. The capture-date sweep next to it was gated this way in #1179; this brings its older twin in line. * fix(admin): gate the dimension status card on the permission the button needs (#1181) Same mismatch as the capture-date card: system.view and system.manage are independent grants and StatusTab renders its card and enabled button purely on a successful status payload (StatusTab.tsx:558), so a system.view-only role got a live Repair button whose every click 403s. * fix(admin): stop the dimension status card polling a 403 (#1181) With the endpoint correctly requiring system.manage, anyone who can open the Status tab but lacks it would have had a 403 and a logged denial every ten seconds for a panel they were never shown. The query is now gated on the same permission the endpoint requires, so it never starts. * fix(admin): gate the dimension card's render on the permission too (#1181) TanStack keeps the cached status after `enabled` flips false, so checking only the payload would still show the card — and an enabled Repair button whose POST 403s — to a lower-privileged admin logging in behind a system.manage user inside the cache lifetime. * fix(admin): name the dimension-card permission flag for the card it gates (#1181) #1179 adds a second system.manage-gated card to this same component with the same flag name. Two identical declarations merge WITHOUT a conflict and then fail to compile — TS2451, cannot redeclare block-scoped variable — and since each PR is green on its own, nothing catches it until main's build breaks. Verified by trial-merging both into main: no conflict, two declarations, tsc fails on both lines. Naming this one for the card it gates removes the trap; once both have landed the two flags can collapse into one. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-26 08:54:04 +02:00
Paul Nothaft	51d20c5920	fix(gallery): show other guests' colour labels in the grid (#1178) (#1180) ... * fix(gallery): show other guests' colour labels in the grid (#1178) A colour set by one guest was visible to others in the lightbox and invisible on the tile. The lightbox reads /photos/:id/feedback, which returns per-colour tallies across everyone; the grid reads /photos, whose payload carried only `my_color_label` — so PhotoCard could render nothing else. The feature simply was not extended to the grid. /photos now also returns `other_color_labels`: the DISTINCT colours other viewers put on each photo, gated on show_feedback_to_guests like every other aggregate. `my_color_label` stays ungated, because a viewer's own selection is not shared data — that distinction is unchanged. Distinct colours rather than counts, and capped at three dots: a tile has room for a couple of marks, and "who marked this, and how many" is a question the lightbox already answers properly. The viewer's own colour is excluded from the dots so the badge and the dots never say the same thing twice, and they sit in opposite corners so they do not read as one group. The inset ring stays the viewer's own signal, which is what the badge was built for. Not addressed: the same issue asks for an identity-less shared colour tag — one tag per photo that any guest can overwrite. Neither existing identity mode does that (`simple` scopes by device fingerprint, `guest` by guest_id), so it is a third model touching the feedback schema, the per-guest caps, moderation and the admin aggregates. That is a feature with its own design, not part of this fix. * fix(gallery): carry other guests' labels into the premium and story grids too (#1178) PhotoCard was not the only place the badge renders. GalleryPremiumLayout and StoryPhotoCard have their own copies, and both still passed only my_color_label — so the fix would have covered the default grid and left the two full-bleed layouts showing nothing, which is the same shape of gap the original bug had. Found by driving a real gallery rather than reading the diff: the masonry grid rendered the dots correctly, and a grep for the remaining call sites turned up these two. * fix(gallery): keep the other-viewers colour dots out of the contested corner (#1178) The dots were placed bottom-left, which is the busiest corner in every layout: Timeline paints a timestamp chip there on every tile, and Grid, Mosaic and Masonry a media-type badge. All of them render after the badge, so the dots sat underneath them. Moved into a single row in the corner the colour-label dot already owns, next to the viewer's own mark. Nothing new is contested, and the grouping reads better anyway — your mark and everyone else's are the same kind of information. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-26 08:53:52 +02:00
Paul Nothaft	849a5807b7	fix(admin): make "Storage used" report storage used (#1164) (#1170) ... * fix(admin): make "Storage used" report storage used (#1164) The tile summed photos.size_bytes — the catalogued size of the ORIGINALS, which has no relationship to the disk PicPeak runs on. In reference mode those files are never copied and sit on the NAS; duplicate rows counted the same file twice (#1162); and it ignored everything PicPeak genuinely does write locally: thumbnails, previews, hero renditions, watermarks and the per-event download cache. The reporter's tile read ~80 GB against 21 GB of real usage. Worse than the label: the same number drove the storage soft-limit warning bar and, via /storage/info, the recommended soft limit — so a reference-mode install got a disk-capacity recommendation computed from bytes that are not on the disk. - new localStorageUsage service walks the storage root and reports the total plus a breakdown. Walking rather than summing DB columns is the point: thumbnail/preview/hero rows record a key and never a byte count, and orphans from a deleted event or an interrupted import are real bytes. Symlinks are not followed, so a link into the media mount cannot put the NAS back in the total. Cached for 5 minutes, since the dashboard polls. - the dashboard tile and /storage/info now report that, with the catalogued figure kept and labelled as such next to it. A failed measurement reads as "unavailable" rather than substituting a number that means something else. On the local rig: 64.37 MB used against 15.75 MB catalogued, of which 27.9 MB is watermarks and 6.8 MB is download cache — none of which the old figure could see. Not addressed here: `.download-cache/all.zip` still has no TTL or size cap. It is now at least visible in the breakdown, which is what makes the case for capping it. * fix(admin): exclude the media share from local storage usage (#1164) External review found the walk could reintroduce the exact over-count it replaces. EXTERNAL_MEDIA_ROOT's compose default is `<storage>/external-media`, where the NAS is bind-mounted. That is a plain directory, not a symlink, so the symlink guard did not cover it and the walk descended into the share — putting every referenced original back into a figure whose whole purpose is to leave them out, and comparing NAS bytes against statfs() of the local disk. On the reference-mode installs this issue is about, that is the failure mode reappearing inside its own fix. The configured root is now skipped when it lies inside the storage root, and the result reports which path was excluded. A directory that merely shares the name is still counted, because those really are local bytes. Also from the review: - concurrent cold-cache callers now share one walk. /dashboard/stats, /storage/info and the sidebar are routinely requested together, and each was starting its own stat-per-file traversal of the whole library. - storage_partial is surfaced in the StorageInfo type and the sidebar tile, not just the dashboard and analytics cards. An unreadable subtree makes the total a floor, and a floor silently compared against a soft limit reads as "safely under". * fix(admin): do not report a disk walk on an S3 backend (#1164) Second review round. S3 installs were regressed. With STORAGE_BACKEND=s3 the originals, renditions, archives and download caches are objects in the bucket and STORAGE_PATH holds only incidental local files — so the walk reported near-zero and the soft-limit recommendation was derived from it. Those installs now keep the catalogued figure, which is the approximation they had before this PR, and the response says which measurement it is (`storage_measurement: 'disk' | 'catalog'`) so the UI labels it instead of implying a disk measurement that never happened. The Settings → Status storage card ignored storage_partial, formatting a lower bound as exact and deriving the limit percentage from it — so an unreadable subtree could read as safely under the limit. It now carries the same `+` marker as the sidebar and dashboard. * fix(admin): stop rendering an absent measurement as zero usage (#1164) Third review round, two findings. The analytics storage bar coerced a null measurement to 0, drawing an empty bar labelled "0% of limit" and suppressing the over-limit state — reading as plenty of room at exactly the moment nothing is known. It now shows the catalogued figure on S3, where that IS the available answer, and says "no measurement available" rather than inventing a percentage when there is none. /storage/info walked the filesystem before checking the backend and then threw the result away on S3. The sidebar polls that endpoint, so a migrated install still holding a large local tree paid a full stat-per-file traversal on every cold cache for nothing. Gated before the walk, as the dashboard route already was. * fix(admin): tell "no disk to measure" apart from "the measurement failed" (#1164) External review of the stable twin. Both were reported as `storage_measurement: 'catalog'`, so a failed local walk made the dashboard claim the objects live in S3. They are different things — one is a fact about the install, the other is a fault — and there is now an `unavailable` state for the second. The analytics percentage could reach the billions. `safeSoftLimit` fell back to `storageUsed || 1`, and on S3 that is null → 1, while the figure beside it came from `catalogedBytes`. An editor or viewer holds `analytics.view` but not `settings.view`, so `/storage/info` 403s for them and `storageInfo` is undefined — which is exactly when that fallback fires. It now falls back to the measured figure, and suppresses the percentage entirely when there is no real limit rather than dividing usage by itself and always reading 100%. Also lands the AnalyticsPage half of the previous round, which the commit message claimed but the commit did not contain — only its backend counterpart was staged. The stable twin has carried it since it was written, so this is the parity gap in the unusual direction. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-26 08:53:39 +02:00
Paul Nothaft	1366d6d14c	fix(previews): preserve alpha and animation in the preview tier (#1171) ... * fix(gallery): stop the lightbox loading originals to display a photo (#1166) The lightbox read `preview_url`, which the server only emits once an admin has flipped lightbox_preview_enabled — off by default. So a stock install fell straight through to `url`, the untouched original: a reporter measured 16.5 MB for a photo whose preview is 345 KB. The lightbox renders its neighbours too, so opening one photo pulled three originals. `slideshow_url` is the same /preview/:id URL, watermark query included, and has been emitted unconditionally for images since #1015 — the slideshow never had a fallback worth taking. Preferring it fixes every existing install with no migration and no admin action, and `url` still backstops videos, where both derivative URLs are null. Verified on the local rig with the toggle off, so the photos API returns preview_url: null exactly as filed. Opening one photo: before GET /photo/82, /photo/81, /photo/21 (3 originals) after GET /preview/82?w=1280, /preview/81, /preview/21 397 KB -> 23 KB per image on that gallery's test photos. The toggle no longer decides whether the lightbox uses previews, so its copy said something untrue; it now describes what it still does, which is pre-generate rather than wait for the first guest to open a photo. Updated in en/de/fr/sl, the locales that carry those keys. * fix(gallery): cover the layouts the lightbox fix missed (#1166) External review found the fix was incomplete, and the review of it found one more. Premium galleries were untouched. PhotoGridWithLayouts returns early for gallery-premium, which builds its own yet-another-react-lightbox slides with `src: photo.url` — so those galleries kept pulling full originals and the reported bandwidth problem remained. They now use lightboxImageUrl for the display source; `download` deliberately stays on photo.url, because what a guest saves must be the original. The Story layout was worse, and neither the issue nor the review caught it: StoryPhotoCard rendered the full original as its GRID TILE, at object-cover in a small card. That is the one place where "hundreds of megabytes for a gallery" was literally true. It now uses the per-device thumbnail tier like PhotoCard, and its PhotoSwipe source uses the preview tier. Animated GIFs keep the original. generatePreviewImage always encodes JPEG, so routing an animated source through the preview tier would have replaced the animation with its first frame — a regression the toggle-off default never had. Animated WebP has the same problem and cannot be distinguished by MIME alone; that needs the backend to report it (Sharp's `metadata.pages > 1`) and is left rather than costing every static-WebP gallery the bandwidth fix. The settings copy claimed too much. "Pre-generate lightbox previews" does not generate anything on save — it unlocks the regenerate button and keeps preview_url emitted. Reworded to say that, in en/de/fr/sl. Not changed: the review's P1 said this bypassed the secure-image route on enhanced/maximum galleries. It does not. AuthenticatedImage collects requiresToken and secureUrlTemplate into an explicitly-voided unusedProps and never substitutes {{token}}, so on those protection levels photo.url was a literal `.../secure/82/{{token}}` that returns 400 — the lightbox was falling back to the 300px thumbnail, not to a protected image. Verified against a live maximum-protection gallery. Codex withdrew the finding on that evidence. * fix(gallery): keep premium downloads working and story framing intact (#1166) Second review round, three findings — two of them regressions this PR introduced. Premium Download became a no-op. handleDownloadFromLightbox recovered the photo with `filteredPhotos.find(p => p.url === slide.src)`, and slide.src is a derivative now, so the lookup found nothing and the button silently did nothing. The slide carries the photo id and the handler resolves by that; what Download hands over is still the original. Story cards were reframed. thumbnail_fit is seeded to 'cover' on every install, so thumbnails are square centre-crops — and story cards are not square (400x500 in the carousel, fixed-height in the desktop grid), so the card's own object-cover cropped them a second time and every photo shifted. They now use the preview tier, which is fit:'inside' and therefore the whole frame: the card looks exactly as it did before, without pulling an original. APNG joins the animated-format guard. It declares image/apng and the preview route would serve a static frame. Animated WebP still cannot be detected from MIME and remains the documented gap. * fix(gallery): keep PNG on the original, alpha and all (#1166) Third review round. generatePreviewImage encodes JPEG, which drops ALPHA as well as animation — a transparent PNG came back flattened against a solid background. And an APNG is normally reported as image/png, so the image/apng check alone missed the common upload path. PNG now stays on the original: it is where transparency is the norm, and rare enough in an event gallery that the bandwidth given up is small. Animated or alpha WebP still cannot be detected from MIME and remains the documented gap; it needs the backend to report Sharp's `pages`/`hasAlpha`. Two further findings are acknowledged and deferred rather than fixed here: - Story cards now request /preview on mount, so a cold gallery generates its previews in one burst. That is a new CPU cost, not a regression — those cards previously fetched full ORIGINALS on mount, which is strictly worse. Doing it properly means viewport-gating AuthenticatedImage, which is a change to a component every gallery surface uses and belongs in its own PR. - The premium layout memoizes slide URLs, so rotating the device before opening the lightbox can leave a photo on the tier chosen for the old geometry. The result is a slightly undersized image, and the fix is a resize subscription this PR does not otherwise need. * fix(gallery): load Story images on approach, and give the hero its own tier (#1166) Every card in a Story gallery mounts at page load — `whileInView` gates the animation, not the render — and AuthenticatedImage fetches from an effect on mount, so all of them requested at once. That was tolerable while they pointed at photo.url, because nothing was generated; pointing them at the preview tier meant a gallery with cold previews would Sharp-decode every original in one burst. The image now waits until the card is within 200px of the viewport, using framer-motion's useInView — the same observer the entrance animation already relies on — with `once` so a card never unloads on scroll-away. Verified on a 62-photo Story gallery: 3 images fetched at load, growing to 15 as you scroll, where all 62 would have fired before. While confirming that, the hero turned out to be doing the same thing the cards were. StoryHero rendered photo.url as a full-bleed object-cover background — a full original on the critical path for first paint of every Story gallery — when hero_url exists for exactly this and is a 1920x1080 cover crop emitted unconditionally for every photo (gallery.js:1139). That gallery now issues no /photo/ request at all: hero_url for the hero, the preview tier for the cards, and only as they come into range. * fix(previews): preserve alpha and animation in the preview tier Follow-up to #1166, which had to bypass the preview tier for GIF, APNG and PNG to avoid a visible regression. This removes the cause. generatePreviewImage encoded JPEG unconditionally. JPEG has no alpha channel and no second frame, so a transparent PNG came back flattened onto a solid background and an animated GIF came back as its first frame — for every consumer of this tier, not just the lightbox: the slideshow (#1015), admin previews, and the face avatars that read it as a whole-frame rendition. It was only invisible by default because the lightbox served originals. Sources with alpha, or more than one page, are now encoded as WebP, which carries both and is still far smaller than the original. Ordinary photos stay JPEG — the common path pays nothing. Two things had to move with it: - The output extension now matches what was written. A PNG source previously produced `preview_foo.png` holding JPEG bytes; harmless while the route hard-coded image/jpeg, wrong once the encoding varies. Existing keys keep working — they are still JPEG and still served as such. - The preview route derives Content-Type from the key. With `nosniff` set, mislabelling would show a broken image rather than being silently corrected. The watermark branch re-encodes to JPEG, so it labels itself explicitly; preserving animation through the watermark compositor is a separate problem. The frontend guess-by-MIME goes away entirely — including the case it could never get right, since a still and an animated WebP declare the same type. Verified on the local rig: a transparent PNG round-trips as `Content-Type: image/webp`, `hasAlpha: true`, 8.3 KB; an ordinary photo still serves `image/jpeg` from a `.jpg` key. * fix(previews): retire the legacy preview keys, and stop mislabelling watermarked ones External review of the stable twin found two defects, both on this branch too. Legacy keys collide with the new naming. The old generator kept the SOURCE basename verbatim while always writing JPEG, so a `.webp` upload produced `previews/preview_shot.webp` holding a JPEG. My PR body claimed "pre-existing keys have no .webp suffix and are JPEG" — that was simply wrong. The route now derives Content-Type from the key and the response carries nosniff, so every photo uploaded as WebP would have rendered as a broken image in the lightbox. Legacy `.png` keys are wrong the other way: flattened JPEGs of what may have been transparent sources, which isPreviewValid would have let stand forever. Migration 188 clears photos.preview_path outright — all of it, not just the suspicious extensions, because a `.jpg` key can equally be a flattened rendition and nothing in the key says so. Previews regenerate lazily on next view under the new encoder, so the cost is one regeneration per photo actually viewed. Storage is untouched, as elsewhere. The watermark branch mislabelled its output. applyWatermark PRESERVES the source format (watermarkService.js:200-211: png stays png, webp stays webp), and its input is the preview — so the output already matches the key the header was derived from. Forcing image/jpeg mislabelled every watermarked WebP preview, and nosniff means the browser would not correct it. The override is gone; the animation loss through the compositor is documented where it happens. * fix(gallery): make the Story hero fix actually work on external galleries (#1166) External review of the stable twin, both applying here too. hero_url was inert for external media. ensureHeroImage only ever called resolvePhotoStorageKey, which returns null for external/reference photos by design — and that null was handed straight to withLocalCopy, which throws, so the hero route caught it and redirected to the full ORIGINAL. #1078 fixed exactly this shape for ensurePreviewImage and nobody carried it across. It stayed invisible until this PR pointed the Story hero at hero_url: on a managed gallery that is a real saving, on a reference-mode gallery it quietly changed nothing. ensureHeroImage now has the same external branch ensurePreviewImage does — direct fs read, per-photo output basename — and returns null instead of throwing for a reference-mode row with no source_origin. The format bypass trusted mime_type, which is not trustworthy here. Migration 039 backfilled every pre-existing photo to image/jpeg regardless of what it was, and adminExternalMedia inserts rows with no mime_type at all — so a mislabelled PNG sailed past the guard and came back flattened. It now checks the filename extension as well. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-26 08:53:05 +02:00
Paul Nothaft	77953c15c1	fix(gallery): stop the lightbox loading originals to display a photo (#1166) (#1169) ... * fix(gallery): stop the lightbox loading originals to display a photo (#1166) The lightbox read `preview_url`, which the server only emits once an admin has flipped lightbox_preview_enabled — off by default. So a stock install fell straight through to `url`, the untouched original: a reporter measured 16.5 MB for a photo whose preview is 345 KB. The lightbox renders its neighbours too, so opening one photo pulled three originals. `slideshow_url` is the same /preview/:id URL, watermark query included, and has been emitted unconditionally for images since #1015 — the slideshow never had a fallback worth taking. Preferring it fixes every existing install with no migration and no admin action, and `url` still backstops videos, where both derivative URLs are null. Verified on the local rig with the toggle off, so the photos API returns preview_url: null exactly as filed. Opening one photo: before GET /photo/82, /photo/81, /photo/21 (3 originals) after GET /preview/82?w=1280, /preview/81, /preview/21 397 KB -> 23 KB per image on that gallery's test photos. The toggle no longer decides whether the lightbox uses previews, so its copy said something untrue; it now describes what it still does, which is pre-generate rather than wait for the first guest to open a photo. Updated in en/de/fr/sl, the locales that carry those keys. * fix(gallery): cover the layouts the lightbox fix missed (#1166) External review found the fix was incomplete, and the review of it found one more. Premium galleries were untouched. PhotoGridWithLayouts returns early for gallery-premium, which builds its own yet-another-react-lightbox slides with `src: photo.url` — so those galleries kept pulling full originals and the reported bandwidth problem remained. They now use lightboxImageUrl for the display source; `download` deliberately stays on photo.url, because what a guest saves must be the original. The Story layout was worse, and neither the issue nor the review caught it: StoryPhotoCard rendered the full original as its GRID TILE, at object-cover in a small card. That is the one place where "hundreds of megabytes for a gallery" was literally true. It now uses the per-device thumbnail tier like PhotoCard, and its PhotoSwipe source uses the preview tier. Animated GIFs keep the original. generatePreviewImage always encodes JPEG, so routing an animated source through the preview tier would have replaced the animation with its first frame — a regression the toggle-off default never had. Animated WebP has the same problem and cannot be distinguished by MIME alone; that needs the backend to report it (Sharp's `metadata.pages > 1`) and is left rather than costing every static-WebP gallery the bandwidth fix. The settings copy claimed too much. "Pre-generate lightbox previews" does not generate anything on save — it unlocks the regenerate button and keeps preview_url emitted. Reworded to say that, in en/de/fr/sl. Not changed: the review's P1 said this bypassed the secure-image route on enhanced/maximum galleries. It does not. AuthenticatedImage collects requiresToken and secureUrlTemplate into an explicitly-voided unusedProps and never substitutes {{token}}, so on those protection levels photo.url was a literal `.../secure/82/{{token}}` that returns 400 — the lightbox was falling back to the 300px thumbnail, not to a protected image. Verified against a live maximum-protection gallery. Codex withdrew the finding on that evidence. * fix(gallery): keep premium downloads working and story framing intact (#1166) Second review round, three findings — two of them regressions this PR introduced. Premium Download became a no-op. handleDownloadFromLightbox recovered the photo with `filteredPhotos.find(p => p.url === slide.src)`, and slide.src is a derivative now, so the lookup found nothing and the button silently did nothing. The slide carries the photo id and the handler resolves by that; what Download hands over is still the original. Story cards were reframed. thumbnail_fit is seeded to 'cover' on every install, so thumbnails are square centre-crops — and story cards are not square (400x500 in the carousel, fixed-height in the desktop grid), so the card's own object-cover cropped them a second time and every photo shifted. They now use the preview tier, which is fit:'inside' and therefore the whole frame: the card looks exactly as it did before, without pulling an original. APNG joins the animated-format guard. It declares image/apng and the preview route would serve a static frame. Animated WebP still cannot be detected from MIME and remains the documented gap. * fix(gallery): keep PNG on the original, alpha and all (#1166) Third review round. generatePreviewImage encodes JPEG, which drops ALPHA as well as animation — a transparent PNG came back flattened against a solid background. And an APNG is normally reported as image/png, so the image/apng check alone missed the common upload path. PNG now stays on the original: it is where transparency is the norm, and rare enough in an event gallery that the bandwidth given up is small. Animated or alpha WebP still cannot be detected from MIME and remains the documented gap; it needs the backend to report Sharp's `pages`/`hasAlpha`. Two further findings are acknowledged and deferred rather than fixed here: - Story cards now request /preview on mount, so a cold gallery generates its previews in one burst. That is a new CPU cost, not a regression — those cards previously fetched full ORIGINALS on mount, which is strictly worse. Doing it properly means viewport-gating AuthenticatedImage, which is a change to a component every gallery surface uses and belongs in its own PR. - The premium layout memoizes slide URLs, so rotating the device before opening the lightbox can leave a photo on the tier chosen for the old geometry. The result is a slightly undersized image, and the fix is a resize subscription this PR does not otherwise need. * fix(gallery): load Story images on approach, and give the hero its own tier (#1166) Every card in a Story gallery mounts at page load — `whileInView` gates the animation, not the render — and AuthenticatedImage fetches from an effect on mount, so all of them requested at once. That was tolerable while they pointed at photo.url, because nothing was generated; pointing them at the preview tier meant a gallery with cold previews would Sharp-decode every original in one burst. The image now waits until the card is within 200px of the viewport, using framer-motion's useInView — the same observer the entrance animation already relies on — with `once` so a card never unloads on scroll-away. Verified on a 62-photo Story gallery: 3 images fetched at load, growing to 15 as you scroll, where all 62 would have fired before. While confirming that, the hero turned out to be doing the same thing the cards were. StoryHero rendered photo.url as a full-bleed object-cover background — a full original on the critical path for first paint of every Story gallery — when hero_url exists for exactly this and is a 1920x1080 cover crop emitted unconditionally for every photo (gallery.js:1139). That gallery now issues no /photo/ request at all: hero_url for the hero, the preview tier for the cards, and only as they come into range. * fix(gallery): make the Story hero fix actually work on external galleries (#1166) External review of the stable twin, both applying here too. hero_url was inert for external media. ensureHeroImage only ever called resolvePhotoStorageKey, which returns null for external/reference photos by design — and that null was handed straight to withLocalCopy, which throws, so the hero route caught it and redirected to the full ORIGINAL. #1078 fixed exactly this shape for ensurePreviewImage and nobody carried it across. It stayed invisible until this PR pointed the Story hero at hero_url: on a managed gallery that is a real saving, on a reference-mode gallery it quietly changed nothing. ensureHeroImage now has the same external branch ensurePreviewImage does — direct fs read, per-photo output basename — and returns null instead of throwing for a reference-mode row with no source_origin. The format bypass trusted mime_type, which is not trustworthy here. Migration 039 backfilled every pre-existing photo to image/jpeg regardless of what it was, and adminExternalMedia inserts rows with no mime_type at all — so a mislabelled PNG sailed past the guard and came back flattened. It now checks the filename extension as well. * test(gallery): the hero fixture follows the root-relative relpath contract (#1166) external_relpath has been resolved from EXTERNAL_MEDIA_ROOT rather than from event.external_path since #1163 landed. This fixture still carried the base-relative form — its own comment noted the change was 'a separate stack' — so the two tests stopped resolving and ensureHeroImage returned null the moment that stack merged. The production path was never affected. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-26 08:46:29 +02:00
Paul Nothaft	05e23ef1a1	fix(admin): move the maintenance sweeps' run state into the database (#1181) (#1184) ... Both photo sweeps tracked whether they were running in a module-level variable. Correct on one replica, wrong behind a load balancer: the status poll answers from whichever process it reaches, so an idle replica reports isRunning false while another is mid-run, the UI re-enables the button, and the next POST lands elsewhere and starts a second pass over the whole library. The .whereNull() guards mean nothing is corrupted; the cost is duplicated S3/NAS I/O and an operator who cannot tell whether a job is running. Migration 189 adds one row per job. The claim is a conditional UPDATE whose affected-row count is the answer — the shape backgroundProcessor already uses to hand a photo to exactly one worker — so two replicas cannot both match. The lease is fenced on a per-claim token: taking over a stale claim does not stop the old runner, so without fencing a superseded runner finishing late cleared the new owner's flag and overwrote its result. heartbeat() reports renewal failure and the loops stop on it. Renewal runs on a timer spanning the claim through release, including the candidate query, because one hung NAS read can outlast the stale window inside a single iteration. maintenance_jobs is excluded from .picpeak archives — an archive taken mid-sweep would otherwise restore a live lease with no runner to release it. The importer filters the same set, so older archives are skipped too. Response shape is unchanged, so the frontend needs no change. Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-26 08:45:41 +02:00
Paul Nothaft	410b8f8f6f	fix(external-media): record capture dates on import, and backfill existing libraries (#1172) (#1179) ... * fix(external-media): record capture dates on import, and backfill existing libraries (#1172) External imports never read EXIF, so photos.captured_at stayed NULL for every row they created. The gallery sorts "Date Taken" with COALESCE(captured_at, uploaded_at), which on a bulk import is the import timestamp — so the sort silently degraded into "order by import batch" with no error and nothing in the UI to say the sort key was missing. The reporter's 12-day trip came back with its first two days at positions 4204-5296 of 5555, because those folders happened to be imported second. - the import reads the capture date next to the sharp().metadata() call that already opens the file, so this costs one more read of the same source rather than a second pass over the mount. Best-effort like the dimensions: a source without EXIF imports with captured_at NULL, as before. - POST /api/admin/photos/repair-capture-dates backfills existing libraries, modelled on the dimension repair beside it — background pass, in-flight guard, status endpoint, and resolvePhotoFilePath, which is what reaches an external row at all. Not a migration: the originals sit on a mount that may be down at upgrade time, reading 8000+ of them would block the boot, and a run that found nothing has to be repeatable. - "no EXIF date" is counted separately from "could not read the file". An operator needs to tell "these files carry no date" from "the mount is broken" before deciding to re-run. - the update is guarded whereNull, so an import finishing mid-run is not overwritten by a slower pass. - every sort branch now carries photos.id as a tiebreaker, not just capture_date. A bulk import writes hundreds of rows inside one second, so uploaded_at and the COALESCE fallback both collapse and the grid reshuffles between loads. id is insertion order, which makes the fallback meaningful. Not addressed: extractCaptureDate reads no OffsetTimeOriginal, and exifr resolves a naive EXIF timestamp against the HOST timezone — so captured_at is not a true instant, and the same file imported on two machines yields two values. That predates this and applies to managed uploads equally; the tests here deliberately assert ordering rather than an absolute instant so they do not encode the bug. Worth its own issue. * fix(capture-dates): read managed originals through storage, skip archived, claim the run flag (#1172) Four holes in the backfill endpoint, all found in review: - Managed photos were resolved with resolvePhotoFilePath, which builds a STORAGE_PATH filesystem path. On an S3 install nothing is there, so every managed row failed. Now split the way the thumbnail regenerator does: external rows read from the mount directly, managed rows go through resolvePhotoStorageKey + withLocalCopy. - Archived events keep their photos rows but their originals are deleted on archive, so those rows failed every run and kept the button lit forever. Excluded from both the job and the status counts. - isRunning was claimed after the candidate query, so two concurrent POSTs could both pass the guard and start a pass. Claimed before the await, with every early exit releasing it. - The noExif comment promised a distinction extractCaptureDate does not make (it returns null for unreadable files too). Reworded to what it is. * chore: drop a stray node_modules symlink committed by mistake The .gitignore pattern is `node_modules/`, which matches a directory and not a symlink of the same name, so a local convenience link slipped past it. It pointed at an absolute path on one machine and would dangle everywhere else, breaking `cd backend && npm install`. * fix(capture-dates): gate the backfill as system maintenance, stop overstating the counters (#1172) The endpoint walks every event in the install and rewrites their metadata, but required only photos.edit — which the built-in team_photographer preset holds (175_granular_permissions_and_presets.js:106). That role exists for a contributing shooter, who should not be able to start a whole-library S3/NAS scan or touch another owner's photos. Now system.manage, with the status endpoint on system.view so the panel simply stays hidden for everyone else. The "without EXIF date" wording also promised a distinction the code does not draw: extractCaptureDate returns null for an unparseable file as well as for one that genuinely carries no date, so both land in that bucket. Reworded to "no date found" / "unreachable" in en, de and fr, which is what the two numbers actually separate. * docs: point the permission note at the follow-up PR (#1172) The dimension repair's matching gate landed in #1182, so the comment no longer needs to describe it as unaddressed. * fix(i18n): align the Slovenian capture-date wording with the other locales (#1172) sl was missed when the counters were reworded from 'without EXIF date' / 'unreadable' to what they actually measure. * fix(capture-dates): gate the status card on the permission the button needs (#1172) system.view and system.manage are independent grants, and StatusTab has no permission gate of its own — a successful status payload is what renders the card and its enabled button (StatusTab.tsx:637). Gating the status endpoint on system.view therefore handed a system.view-only role a live Backfill button whose every click 403s, with no error surfaced by the mutation. The comment above it already claimed this endpoint matched the POST. Now it does. * fix(gallery): make the Date Taken sort correct on SQLite (#1172) photos.captured_at does not hold one type on SQLite. Three writers put three different things in it: integer managed uploads — photoProcessor.js:488 hands knex a Date, which the sqlite3 binding stores as epoch milliseconds text external imports and the backfill, which write ISO-8601 null no capture date, so the sort falls through to uploaded_at, itself text in knex's 'YYYY-MM-DD HH:MM:SS' default shape A plain COALESCE over that is not an ordering. SQLite sorts INTEGER before TEXT unconditionally, so every managed photo carrying EXIF came back ahead of every photo that did not, whatever the dates said — a 2027 capture landing before a 2020 one. Among the text values 'T' (0x54) also outranks the space (0x20), so a same-day ISO 01:15 sorted behind a fallback 23:00. Both failures predate this branch — the first needs only two managed photos — but making that sort correct is what #1172 is about, so it is fixed here rather than left for the issue it belongs to. Normalised in the ORDER BY rather than by rewriting the column: the data fix would have to touch every existing row and every writer, which is a far heavier change than the sort it corrects. The cost is that this sort no longer uses idx_photos_captured_at on SQLite — an acceptable trade on the fallback engine, where the alternative is an index-assisted wrong answer. Postgres is untouched: captured_at is a real timestamp there and COALESCE already compares correctly. The regression tests drive the real gallery route on real SQLite. They write the epoch-millisecond integer directly, because the Date that produces it in production cannot be reproduced inside jest — there the binding's type dispatch misses sandbox Dates and stores "[object Object]" (CLAUDE.md). All four behavioural tests fail on the unfixed ORDER BY; verified by reverting it. * fix(gallery): normalise epoch-integer uploaded_at too, and stop polling a 403 (#1172) Two follow-ups from review. uploaded_at is not always text on SQLite either. A legacy archive restore leaves epoch milliseconds in it — there is a test pinning exactly that (__tests__/integration/sqliteEpochTimestamps.test.js) — and the fallback branch read it with substr(), so '1830297600000' was compared against '2020-01-01 00:00:00' as text and a 2028 upload sorted first. Both columns now get the integer/real branch. The status card also polled every ten seconds regardless of permission. With the endpoint correctly requiring system.manage, anyone who can open the Status tab but cannot run the job would have had a 403 and a logged denial every ten seconds for a panel they were never shown. The query is now gated on the same permission the endpoint requires, so it never starts. * style: quote convention in the capture-sort test (#1172) * fix(capture-dates): skip watcher-imported videos, and make the status counts consistent (#1172) Three follow-ups from review. fileWatcher.processNewPhoto sets type='video' and a video/* mime but never media_type (fileWatcher.js:128-130), so those rows keep the 'image' default from migration 048. Filtering on media_type alone queued every such video on every run — extractCaptureDate returns null for a video, captured_at stays null, and the backlog never cleared. Candidate query and status scope now check all three markers. The status counts were two separate queries, so an import committing a dated photo between them could be counted by the second and not the first: the card then showed withCaptureDate > total and a negative backlog, with the button enabled to "fix" it. One aggregate now. And the card's render checked only the cached payload. TanStack keeps that after `enabled` flips false, so a lower-privileged admin logging in behind a system.manage user inside the cache lifetime would still have seen the card and a button whose POST 403s. The permission is part of the render condition now. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-08-26 08:43:13 +02:00