79eb6d72eb170ab4e7f4bf33d7cf2d97cd5fbaa8
146
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Paul Nothaft
|
1e8b6f1b0f |
fix(usage): close the QA findings on opt-in product usage
A QA exploration of this branch against an isolated rig — own stub collector, SQLite and PostgreSQL — turned up one dead end and a set of signals and controls that did not hold up. This closes all of them. Rotating JWT_SECRET, the documented response to a suspected compromise, made the signing key unreadable. That was already named and documented, but it left no way out: the delete packet can never be signed, so the row stays deletion_pending forever, and enable() refuses because it is not `disabled`. An operator who rotated precisely because the secret was compromised cannot restore it, so the feature was bricked with no control left. POST /usage/abandon is offered only in that state; it drops the local identity and records the receipt as `collector-unconfirmed` rather than claiming a deletion that did not happen. Every failed delivery was retried on the next admin request, and /activity is open to any authenticated admin while the settings ticker fires it every five minutes per open tab — 30 activity calls against a rejecting collector produced 30 outbound requests. Migration 206 adds attempts/next_attempt_at and the unattended sender honours the gate; Retry and opt-out still send immediately, and the tab names the time of the next automatic attempt. Feedback, votes and portal sessions now share an installation-wide budget of 30/hour. They are the only endpoints whose effect is outbound traffic carrying operator-written free text, and the general limiter skips authenticated requests by design. Reading status and withdrawing stay unthrottled. gallery_image_protection was true on a bare install with no galleries: PicPeak ships default_protection_level='standard' and enable_devtools_protection=true, so it reported fleet-wide 100% and could never separate a decision from an untouched default. It now reads only what deviates from the shipped defaults, and the devtools flag is not read at all — being on by default, its only informative state is off, which is the opposite of what the key claims. Also: - the export receipt counted every packet and called the total "usage reports"; reports and participant operations are now counted and named separately - GET /usage/preview no longer persists the custom_css marker, so the transparency view stops changing what will be sent - the feedback route requires every field the packet schema requires, so an API caller gets the missing field named instead of a bare INVALID_PACKET from inside signing - the German strings for this feature use "Sie" throughout, matching the rest of the admin UI; the ignore hint says what ignoring will do rather than stating it as already true - the consent dialog returns focus to the control that opened it - the long buttons wrap instead of running off a 390px viewport - a deletion receipt is labelled as belonging to an earlier participation while a new one is active Regression tests cover each of these, including the delete packet's reuse of the last accepted sequence, which was an unwritten assumption about the collector rather than a defect. |
||
|
Paul Nothaft
|
a7382591bf | feat: expand opt-in capability coverage with versioned consent | ||
|
Paul Nothaft
|
5d31b61c8d | Merge remote-tracking branch 'origin/main' into codex/1110-usage-coverage | ||
|
Paul Nothaft
|
e347f8f40f | fix(usage): minimize session receipts and clarify privacy controls | ||
|
Paul Nothaft
|
7ff8caf9d7 |
fix: remove the fragmentation handling stranded by #1303
#1298 and #1303 merged together. #1298 taught the creation paths to resolve a fragmentation_level default; #1303 removed everything that consumed it. Neither conflicted textually, so main ended up validating the field on create and update, copying it on duplicate, resolving default_fragmentation_level for it, and advertising it in the v1 API docs — for a value nothing reads and a setting the Image Security tab no longer exposes. Inert rather than broken, which is exactly why it needed removing on purpose: dead code that contradicts the PR that just deleted the feature is how the next reader concludes fragmentation still works. The events.fragmentation_level column and the app_settings row stay, as #1303 decided — dropping a column is irreversible and the stored values are harmless once nothing reads them. Refs #1300 |
||
|
Paul Nothaft
|
cc263f2e87 |
fix(usage): isolate the Postgres fixture, and stop two more wrong signals
Three findings, one of them mine and CI-affecting. The Postgres suite gets its own schema. CI hands every gated suite the same PICPEAK_PG_TEST_URL and runs jest with parallel workers, and both picpeakRestorePg and externalRelpathFoldPg drop and recreate `events` and `app_settings` in it — so the suite I added would have destroyed their fixtures and vice versa, intermittently. It now creates and drops its own `usage_pg_test` schema and reaches the tables through searchPath, which works because the service queries unqualified names. Verified on a clean database: after the run `public` still holds zero tables. My first attempt at this silently did not apply — the replacement anchor had been reformatted by eslint and I printed success without asserting the match, which is why the first "isolated" claim was wrong. Webhook-only installs are no longer counted as SMTP users. With EMAIL_WEBHOOK_URL and EMAIL_WEBHOOK_SECRET set, adminEmail sends /email/test through the webhook transport and never touches SMTP (#1225 added that path), but the rule recorded the permanent `smtp` marker anyway. Gated on the transport that is actually configured. Activation is written atomically with its acknowledgement. Split across two updates, a failure or a stop between them left the row activation_pending with pending_packet already cleared — registered with the collector, and permanently stuck locally, because tick() has nothing to retry from there. The register case now sets status in the same write and is guarded precisely on activation_pending rather than merely "not withdrawing". Refs #1110 |
||
|
Paul Nothaft
|
c7cedb00d6 |
test(usage): prove product usage works on PostgreSQL, and harden the collector default
Everything about this feature had been exercised on SQLite only, which is the engine least likely to show its problems. Adds __tests__/integration/productUsagePg.test.js, following the gated pattern the .picpeak restore suites use: it runs the real migrations 201-203 against a real PostgreSQL and covers what SQLite cannot answer. node-postgres returns bigint as a STRING, and the withdrawal guard compares `cancel_seq` — a `'1' !== 1` slip there would let an activation complete after an opt-out, and SQLite, which hands back a number, would never show it. Booleans are real booleans rather than 0/1, which is what every `configured` signal in a report is built from. And markUsed takes SELECT ... FOR UPDATE on this engine only. Seven cases, all passing against PostgreSQL 15. Removing the compare-and-swap condition fails the withdrawal case there too, so the suite has teeth on that engine and not only on SQLite. CI already provides PICPEAK_PG_TEST_URL, so these run there rather than skipping. The collector default is harder to lose. An unset, empty or whitespace-only USAGE_COLLECTOR_URL now falls back to https://usage.picpeak.app — deployments that template the variable in (docker-compose writes ${USAGE_COLLECTOR_URL:-...}) can hand over an empty string, and that has to mean "use the default" rather than "no collector". A value that is present but malformed is still reported as a configuration error instead of being silently replaced: quietly retargeting a self-hoster's collector at ours would send their reports somewhere they did not choose. Refs #1110 |
||
|
Paul Nothaft
|
0deef2584f |
fix(security): one settings decoder, and the last creation path
Round-four review follow-ups. Every reader of app_settings now shares decodeSettingValue. The previous commit taught the GET handler to decode, which on a legacy SQLite install made the tab show devtools protection as disabled while readBooleanSetting — parsing once, getting the string 'false', rejecting it — left new galleries with it enabled. A decoder used by only some readers is worse than none, because the UI and the behaviour disagree. readBooleanSetting, getImageSecurityDefaults, the v1 devtools fallback and the settings GET all use it now. Standalone contract conversion covered. contract/conversions.js takes Path B and inserts its own event row when the contract has no source quote, so signed standalone contracts were the last path still landing on the migration-038 column defaults. Refs #1296 |
||
|
Paul Nothaft
|
0e560ebb19 |
fix(security): decode settings at the API boundary and honour the transaction
Round-three review follow-ups. getImageSecurityDefaults now accepts a transaction, the way getAppSetting two lines above it already does. quoteService.convertToEvent called it from inside db.transaction() through the global db; sqlite3 runs a single-connection pool, so that read would have waited on the connection its own transaction was holding until the acquire timeout, and the helper's catch would then have swallowed the error and dropped the defaults silently. The double-encoding is fixed where it starts. GET /admin/image-security/settings returned setting_value undecoded, so it shipped "true" to a tab that types the field as boolean — and since the tab PUTs the whole object back through JSON.stringify, every save wrapped another layer around values nobody edited. It decodes now, so a round trip is idempotent. The tab is the only consumer of that endpoint. The reader unwraps to any depth instead of four. The depth on an existing install is however many times someone opened that tab, which is not a number to cap. It terminates because each parse of a string is strictly shorter than its input. Refs #1296 |
||
|
Paul Nothaft
|
19c518aaa5 |
fix(security): close the remaining image-security default gaps
Round-two review follow-ups. Settings survive the tab round trip. GET returns setting_value without decoding it and ImageSecurityTab PUTs the whole fetched object back through JSON.stringify, so on SQLite one visit to the tab re-encodes every value it read. A single parse then yields the string "true", the type checks reject it, and the defaults go quietly dead — the exact bug this change exists to fix, returning by a different route. The reader now unwraps until the value stops being a JSON string, bounded. Array overrides rejected. express-validator applies isInt/isIn/isBoolean element-wise, so `image_quality: [72]` passed the chain and arrived still an array — a PG insert error, and `[false]` coerced to true by formatBoolean. Both create routes now use .not().isArray(), and the shared resolver ignores non-scalars for any future caller. Two more creation paths covered. quoteService.convertToEvent builds its own events row, so CRM-converted galleries fell back to column defaults. /:id/duplicate copies fifteen source columns including enable_devtools_protection but missed these four, so duplicating a 'maximum' gallery produced a 'standard' one — a duplicate now inherits the source's values, not the current globals, since copying the gallery is the point. The PUT /:id chain has the same array weakness. Pre-existing and outside this fix; left alone deliberately. Refs #1296 |
||
|
Paul Nothaft
|
ab6c33d9eb |
fix(security): apply image-security defaults on every creation path
Review follow-ups on the #1296 fix. The defaults were resolved only in the admin POST / handler. POST /api/v1/events builds its own insert and resolved just the devtools setting, so an API-created gallery still fell back to the column defaults — the same split that made #592 a separate bug from #317, about to be repeated. Both paths now share resolveImageSecurityColumns(). An explicitly supplied value now wins over the global default. The create routes never accepted these four fields at all, though PUT /:id has validated them all along, so a client sending protection_level on create had it silently dropped. The previous comment claimed the spread ordering preserved a request value; there was no request value to preserve, and a later spread would have overridden one anyway. Settings validation no longer leans on parseInt, which rescues '72oops', 72.5 and [72] into valid-looking integers. The settings PUT stores whatever JSON it is handed without validating values, so those really can reach the resolver. fragmentation_level is still stored and consumed by no renderer — ProtectedImage hardcodes a 4-grid and secureImageService a 3x3. Noted in the API docs rather than silently implied to work. Refs #1296 |
||
|
Paul Nothaft
|
8ca3610514 |
fix(security): apply the Image-security defaults instead of storing them (#1296)
Four controls in Settings → Image security were written, reloaded and rendered as toggles, and read by nothing: default_protection_level → events.protection_level default_image_quality → events.image_quality enable_canvas_rendering → events.use_canvas_rendering default_fragmentation_level → events.fragmentation_level Each maps onto a column migration 038 already created, and each is labelled "… by default". `enable_devtools_protection` was the only one of the five ever wired (#317), and its plumbing is the pattern this follows. Reported for enable_canvas_rendering by @leonlivevocalist-svg while instrumenting #1287 — the setting was globally true on their install and zero canvas elements were created. Checking the neighbours found three more of the same, so fixing one and leaving three would have been worse than leaving all four. CREATION-TIME ONLY, deliberately. Applying these to existing events would silently change live galleries on upgrade: an install with enable_canvas_rendering already on would flip every grid to canvas rendering, which is memory-expensive at scale and is the exact profile under investigation in #1287. New events inherit; existing rows are untouched. A missing or malformed value yields no key, so creation falls through to the column default exactly as before — including the ranges, where an out-of-range quality or fragmentation level is ignored rather than clamped into something the operator did not choose. `false` is carried through rather than dropped as falsy, or "off" would be unreachable. The spread sits after the explicit columns so a value supplied by the request still wins. 12 tests: the mapping, the false case, seven malformed inputs falling through, partial configuration, and that a settings failure cannot block event creation. |
||
|
Paul Nothaft
|
fc595409b4 |
feat(crm): newsletter campaigns behind a newsletters flag (#1264)
Part B of #1264. Flag off by default, so an install that never enables it gains no route, no nav entry and no way to mass-mail. A campaign is a body plus a recipient rule. Queueing one writes ordinary email_queue rows (email_type 'newsletter', origin 'campaign', new campaign_id), so retry, rendered_html, sent_at and error_message all come from the existing processor rather than a parallel sender. Throttling staggers scheduled_at; the processor loop is untouched. Two rules the service enforces: no raw HTML is ever stored (sanitized on write and again on render, idempotently), and opt-out is checked at queue time AND again at send time. Migration 199 adds email_campaigns, email_campaign_recipients, email_queue.campaign_id, customer_accounts.marketing_opt_out(_at), and the newsletters.view / newsletters.send permissions. Three rounds of external review are folded in, including several that would otherwise have shipped broken: - Campaign rows never came due on SQLite. queueEmail writes a Date, which the sqlite3 binding stores as epoch ms; ISO text in the same column compares as TEXT against an INTEGER, and SQLite orders every INTEGER below every TEXT. The feature silently sent nothing there. - The flag had no Settings card and no sidebar entry, so it could not be enabled through the UI at all. - Consent is per ADDRESS, not per row: two accounts sharing an inbox meant unsubscribing stopped one and not the other, at both queue and send time. - The unsubscribe GET mutated consent, so a mail-security scanner walking a campaign could have unsubscribed much of the list. GET now confirms, POST acts. - The rate ceiling is clamped to the queue's real throughput (10/min), so the composer's estimate stops being wrong by up to 12x. Closes #1264 |
||
|
Paul Nothaft
|
8f98f6bdec |
fix(gallery): show a guest their own likes when feedback sharing is off (#1286)
show_feedback_to_guests means "don't show guests OTHER PEOPLE's feedback". The per-viewer is_liked flag was gated on it anyway, so turning sharing off emptied every heart the guest had set themselves, on every page load, while the photo_feedback rows sat there intact. The query behind the flag is filtered to the viewer (by guest_id, or by their own IP+UA identifier), so what it returns was never aggregate data. The colour-label block twelve lines below already documents this exact reasoning and is correctly ungated. Scope is just that flag — the counts beside it stay gated, with a test pinning that the fix does not leak them back. The #1150 contract still holds: an admin-hidden like does not read as liked. Note for the reporter: the FILTER path was already correct (includeGuestMatches is ungated, /my-feedback carries no gate). The empty Likes chip was downstream of the same falsified flag, not a second bug. Closes #1286 |
||
|
Paul Nothaft
|
b6e40b9a2a |
feat(email): global signature footer from the business profile (#1264)
The business profile already carried the operator's full issuer block —
address, phone, email, website, VAT id — but none of it reached an email.
Those columns only fed the quote/invoice PDF renderer, so every outgoing
mail footer was the fixed logo + company name + copyright line.
The signature is rendered by wrapEmailHtml and nowhere else, so no
template, no per-type send path and no queue row needed a change. Two new
columns on business_profile (migration 198) carry the toggle and one
free-text legal line; everything else is read from the address fields the
operator already maintains.
Default off, with a test pinning that the disabled path is byte-identical
to a no-profile install.
Includes three rounds of external review fixes: the plain-text MIME part
also carries the signature; string booleans ('false'/'0') no longer
invert the toggle; the status line stays silent rather than asserting
"off" while unauthorised or loading; and the preview's Text tab mirrors
the send path's htmlToText fallback.
Manual Messages replies deliberately keep no signature — they bypass the
wrapper by design — and the UI copy names that exception.
Closes #1264 (Part A)
|
||
|
Paul Nothaft
|
a7d45ddd0d |
fix(workflows): restore the once-per-process seed guard
`booted` was assigned but never read, so the guard's early return was missing and the builtin workflow seeder ran on every call. Impact was wasteful, not harmful: seedOneBuiltin is idempotent -- it keys on builtin_key and returns early when adminOwned or storedVersion >= def.version, writing a graph only on a fresh insert or a version bump. So repeat calls cost a lookup per builtin plus a graph rebuild, with no duplicate rows. `booted = true` stays inside the try, so a seed that never got off the ground (workflows table not migrated, DB down) leaves the flag clear and retries. A per-builtin failure is still swallowed by the inner catch and does not block the flag, unchanged. Restoring the guard broke workflowEngine.test.js, which calls the boot seeder seven times in one worker and needs the second call to run in two of them. Followed the existing _backupPathsBoot/_restoreSettingsBoot precedent: exported _resetBootForTests(). Refs testplan REPORT.md B3. |
||
|
Paul Nothaft
|
3f6c81a846 |
fix(photos): treat category_id 0 as uncategorized instead of storing it
Genuine product bug, found behind the adminPhotos.reference suite (which was
failing for an unrelated reason -- see below).
parseInt('0') is 0 and !isNaN(0) is true, so a '0' category_id was written
literally. photo_categories.id is an increments() column, so 0 can never be a
real category, and every read path already assumes it cannot happen: the list
mapper does `category_id || type` (0 is falsy, renders as uncategorized) and
the list filter explicitly skips '0'. The result was a filter black hole -- the
photo matches no numeric category filter, and misses the "uncategorized"
filter too because that is whereNull(). Displayed as uncategorized, reachable
by nothing.
null rather than a 400: unparseable input ('abc' -> NaN) already falls through
to null, so 400ing on '0' while silently accepting 'abc' would be incoherent,
and '0' is just the HTML <select> shape where the "none" option carries
value="0".
Fixed at all three call sites that share the branch -- PATCH /photos/:photoId,
POST /photos/bulk-update, and the upload route, where the dangling 0 was
written at creation time and the scope-validation guard
(`if (parsedCategoryId && ...)`) skipped on the falsy 0 and let it in
unvalidated. Only the PATCH one was behind the failing test; leaving the other
two would have left the bad state creatable.
The suite's 3 failures were all masked by a fixture gap, not this bug: it
stubs middleware/auth but not middleware/permissions, so requirePermission's
admin_users JOIN roles query hit tables the fixture never creates and every
request 500'd before reaching a handler. Stub it, bring the photos fixture up
to the 7 migrations it had drifted behind, and correct a stale 200 that became
202 when uploads went async in
|
||
|
Paul Nothaft
|
1d84c738d8 |
test: repair four stale backend suites
All four asserted contracts the product has since moved past. No genuine
product bugs behind any of them; assertions were tightened, not loosened.
adminAuth (3 tests): never mounted errorHandler, so ConflictError/
ValidationError arrived as empty Express defaults. The route also checks
username before email, so the "email conflict" fixture was hitting the
username branch. Mount the handler, fix the fixture, match the real response
shapes.
backupService.enhanced (12 tests): three stacked drifts -- the db mock had no
.returning(), so every runBackup threw at the insert; ensureDatabaseDumpForBackup
now lazily requires ./databaseBackup inside the run, which fails under
mock-fs; and the rsync path moved from exec(shell string) to
spawnAsync('rsync', args) with an isHostAllowed SSRF preflight. Also updates
getBackupStatus to its current shape (frontend aliases, nextScheduledRun null
when no schedule is enabled, #871).
adminSettings.logo: POST /logo gained requirePermission('settings.edit');
the hand-rolled db mock returns a bare Promise from select(), so the
permission lookup threw a TypeError into a 500. Mock the permissions
middleware alongside the already-mocked auth.
crmMintPaths (2 tests): macOS-only. The expected prefix was realpath'd while
the services persist under the raw STORAGE_PATH -- identical on Linux CI
(/var vs /private/var only diverges on macOS), which is why it passed there.
The comment justifying the realpath referenced process.cwd() behaviour the
services no longer have.
Refs testplan REPORT.md #22 (Part 1.2.01).
|
||
|
Paul Nothaft
|
c5c5a6b0c8 |
fix(webhooks): write delivery timestamps as ISO strings
Applies the repo's documented Jest+SQLite guidance (CLAUDE.md) to the webhook delivery path, which was the last one still passing raw Date objects into knex writes. Under jest those store as the literal string "[object Object]", so next_retry_at came back NaN and the retry/backoff test could not assert on it. Production (PG, and SQLite outside jest) was unaffected. Convert the timestamp writes -- and the `next_retry_at <=` due comparison, which has to stay type-consistent with them -- to .toISOString(), matching the existing precedent in downloadJobService.js. Refs testplan REPORT.md #22 (Part 1.2.01). |
||
|
Paul NothaftandPaul 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 <[email protected]>
|
||
|
Paul NothaftandPaul 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 <[email protected]> |
||
|
Paul NothaftandPaul 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 [email protected]" 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 <[email protected]> |
||
|
MarianandPaul Nothaft
|
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 <[email protected]> |
||
|
Paul NothaftandPaul 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 [email protected] and [email protected]. /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 <[email protected]> |
||
|
Paul NothaftandPaul 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 <[email protected]> |
||
|
Paul NothaftandPaul 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 <[email protected]> |
||
|
Paul NothaftandPaul 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 <[email protected]>
|
||
|
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. |
||
|
Paul NothaftandPaul 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 <[email protected]> |
||
|
Paul NothaftandPaul 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 <[email protected]> |
||
|
Paul NothaftandPaul 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 <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
a7b74bcd87 |
fix(external-media): store external paths from the media root (#1163) (#1168)
* fix(external-media): store external paths from the media root (#1163) Importing a second folder into an event silently invalidated every photo already in it. photos.external_relpath was stored relative to events.external_path, and every import overwrites that column — so the older rows were rebased onto the new folder and their originals resolved to paths that do not exist. Nothing errored, and the grid still looked intact: thumbnails are written to local storage during the import while the base path is still correct. Only what needs the original broke — preview generation, the lightbox, downloads — which presents as a gallery that looks slow rather than one that is broken. The reporter had 7547 of 8004 rows pointing into the void and spent a while chasing it as a CPU problem. - external_relpath is now relative to EXTERNAL_MEDIA_ROOT, so a row is self-describing and nothing an admin does to the event afterwards can move an already-imported photo. - migration 187 folds each event's base path into its rows. Where the current resolution is missing on disk it walks up the base path for an ancestor under which the file IS there — the already-rebased case — and where it finds nothing it leaves the row resolving exactly where it resolves today. Skipped entirely when the media root is unmounted, since every file looks missing then. - the fold also runs after a .picpeak restore: knex_migrations is excluded from the archive, so a pre-#1163 backup would otherwise land base-relative rows on a migrated instance. - drops the duplicate-leaf-segment guess in photoResolver. It papered over this same double-prefixing and actively corrupts a root-relative path whose first segment legitimately repeats (base 'Trip', row 'Trip/x.jpg'). * fix(external-media): verify provenance and fold atomically (#1163) External review found four real defects in the fold. Repair could adopt the wrong file. Existence alone was accepted as proof that an ancestor candidate was the row's original — so a row whose file an admin simply deleted would adopt any same-named file one directory up (base `Trip/Sub`, relpath `photo.jpg`, an unrelated `Trip/photo.jpg`), and downloads would then serve a different photo. Worse than a dead link. An ancestor must now also match photos.size_bytes, which the import recorded from the very file the row describes; rows carrying no size are never repaired from an ancestor. The CURRENT base is still accepted on existence alone, because nothing is being inferred there — that is where the row already resolves. The fold was not atomic. Every UPDATE committed independently and the marker came last, so a process killed mid-fold left converted and unconverted rows with no marker — and the next run folded the converted ones a second time, putting every original one directory deeper with no undo. Probing is now a read-only first phase (so a slow cold NAS does not hold a write transaction open), and every rewrite plus the marker commit together. Failed rewrites certified a partial conversion. The per-row catch counted any error as a collision, carried on, and wrote the marker anyway — leaving that row in the old format for a resolver that now reads it differently. It also could not tell a genuine duplicate from a SQLite lock or I/O fault. Target collisions are now resolved in the planning phase, where they can be identified honestly, and a write that fails rolls the whole fold back. Restore ordering. The fold ran after the face requeue, with the worker live — so a worker could claim an external row while it was still base-relative, resolve it against the wrong path, and burn it to 'failed', a state only an explicit Re-scan clears. The fold now runs first, for the same reason the requeue already sat after restoreFiles. * fix(external-media): close the fold's remaining stranding paths (#1163) Second review round, three findings. A collision loser was left stranded. When an event imported one file through both `Trip` and `Trip/Sub`, two rows folded to the same path and the loser was skipped — keeping a base-relative value that the root-only resolver then reads as `<root>/<relpath>`, permanently wrong, with the marker claiming conversion was complete. It is a duplicate by construction, so it now goes through migration 186's deleteDuplicatePhotos, which reparents its feedback and marks and reconciles the face clusters instead of orphaning them. This branch is rebased onto #1162 for that helper. The other restore path had the same face-ordering bug. restoreService queued face scans in step 6, before step 7c runs pending migrations — so a pre-187 full or database restore handed the live worker rows whose paths were still event-relative, and it burned them to 'failed', a state the later fold does not clear. The requeue now happens after the migrations, where the files already are. A failed conversion was reported as a clean restore. The fold is transactional, so a failure leaves every external path in the old format under a resolver that reads from the media root — every original unreachable. It was logged as a warning and the restore returned success. It now returns externalPathsConverted/externalPathError, and suppresses the face requeue, which would otherwise mark those photos failed on top. * fix(external-media): make the fold safe against its own intermediate states (#1163) Third review round, four findings. A one-pass rewrite could collide with itself. Every FINAL path is distinct, but a final value can equal another row's CURRENT one — `photo.jpg` repairing to `Trip/photo.jpg` while the row already holding `Trip/photo.jpg` folds deeper — so the update violated migration 186's unique index halfway through. On Postgres that surfaces as 23505, which run-migrations-safe.js mistakes for "schema already exists" and records 187 as applied after the rollback, leaving every path unconverted with nothing to retry. Rows now park on a per-row staging value first, and migration 187 re-throws without the driver's code so the runner cannot misread it. The bulk update targeted rows the plan never saw. Phase 1 probes outside the transaction and can run for minutes; an import finishing in that window inserts an already root-relative row, and `where event_id` prefixed it again with the stale base. It now updates by the ids phase 1 captured. The restore UI never showed a conversion failure. The API carried externalPathsConverted, but PicpeakBackupCard neither declared nor read it and showed a green success either way — so an admin whose external originals were all unreachable was told the restore worked. restoreService requeued faces even when the migrations failed. The step 7c catch is deliberately non-fatal, so a pre-187 backup whose fold never ran still handed the live worker event-relative paths to burn to 'failed'. * fix(external-media): the fold's staging value must be storable on Postgres (#1163) External review of the stable twin caught this, and it was on both branches. The two-pass rewrite parks each row on a temporary value, and that value was written with a leading NUL. SQLite stores NUL in TEXT without complaint; Postgres rejects it outright — "invalid byte sequence for encoding UTF8: 0x00" — so migration 187 rolled back on exactly the installs that need the two-pass repair, and only on the engine most of them run. Restores hit the same wall and reported the conversion as failed. The prefix is ordinary text now. It still cannot collide with a real relative path and is still obviously wrong if a crash leaves one behind. Adds a gated Postgres test alongside the existing picpeakRestorePg one, because a SQLite-only suite structurally cannot catch this class: restoring the NUL makes exactly the two-pass repair case fail with that error, and nothing else. --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
06da1b9f7e |
fix(external-media): one row per external file per event (#1162) (#1167)
* fix(external-media): one row per external file per event (#1162) Two overlapping import-external runs against the same event inserted every file twice. The route checked for an existing external_relpath and then inserted, with an fs.stat and a sharp().metadata() read sitting in between — a window wide enough for both runs to see "not there". A reporter's event held 8004 rows for 6012 distinct paths. Nothing at the storage layer stopped it: migration 041 created only a NON-unique (event_id, source_origin) index. - migration 186 removes the duplicates that already exist and adds a partial unique index on (event_id, external_relpath). The survivor is the lowest id that has a thumbnail, so a half-finished import does not cost a grid tile, and hero references are repointed first because the FK is SET NULL. - the route treats a unique violation as a skip and carries on, so a second writer this process cannot see (another replica) converges instead of duplicating or 500ing. - a second import while one is already running now gets a 409 rather than walking the whole tree to have every insert bounce. The duplicates' thumbnail files are left behind as unreferenced bytes — a migration is the wrong place to reach into storage, which may be S3. * fix(external-media): keep dependent rows and legacy restores intact (#1162) External review found two real defects in the dedupe half of this fix. Dangling rows on SQLite. Every FK into photos declares ON DELETE CASCADE, but PicPeak never sets `PRAGMA foreign_keys = ON` — the codebase says so where it deletes an event (adminEvents/helpers.js:245) — so on every SQLite install the cascade is inert and deleting a duplicate photo left its face embeddings, guest feedback and admin marks behind, pointing at an id that no longer exists. Biometric data outliving its photo is exactly the invariant the event delete goes out of its way to hold. Dependents are now handled explicitly, and moved rather than discarded where they can be: the duplicates were separate tiles in the grid, so a guest's comment or an admin's rating could legitimately be on either, and dropping it inside a fix for silent data loss would be its own bug. Where the target already holds an equivalent row — the same guest's like, the same admin's mark, the same transfer's entry — the loser is dropped, because those tables mean one row per (photo, actor). photo_faces is the deliberate exception: both rows were scanned, so moving would duplicate every embedding and split the person clusters built from them. Legacy restores. Suspending FK enforcement does not suspend a UNIQUE index on either engine, so a .picpeak backup taken before migration 186 — carrying exactly the duplicates it removes — would hit the new index mid-batchInsert and roll the whole restore back, after every table had already been emptied. The restore now drops the index for the load and rebuilds it after running the same dedupe. Also: a failed CREATE INDEX is no longer swallowed. Recording the migration as applied without it leaves the install permanently racy, with nothing to trigger a retry. The shared work moves to services/externalPhotoDedupe.js, which the migration and the restore both call. * fix(external-media): reconcile derived state around the dedupe (#1162) Second review round, four more real findings. The index throw did not actually stop anything. run-migrations-safe.js treats 23505 as "schema already exists" and marks the migration applied (run-migrations-safe.js:138) — and a CREATE UNIQUE INDEX that finds duplicate rows raises exactly 23505 on Postgres. A replica inserting one between the dedupe and the index lock is a real rolling-deploy shape, and the outcome was the thing the throw was added to prevent. The index is now verified against the catalog afterwards, and failure raises a code-less error the runner cannot mistake for idempotence. Two people sharing a device were treated as one. photo_feedback carries both guest_identifier (per device) and guest_id (per person, migration 078), and feedbackService scopes by guest_id when present. Keying equivalence on the identifier alone deleted one of two different people's ratings. It now uses the same COALESCE rule the service does. Deleting faces raw left ghost people. event_people counts and centroids are derived from the photo_faces rows being removed, and #1132's separation snapshots hold a copy of each side's centroid — which is why faceProcessor exposes purgePhotoFaces and says it is "called from every photo-deletion path". The dedupe now goes through it. Reparenting feedback left the survivor's totals stale. photos carries denormalized feedback_count / like_count / average_rating / favorite_count and the later reaction and colour counts, so a survivor that now owns feedback kept rendering zero. updatePhotoFeedbackStats takes a trx so the dedupe can recompute on its own connection. Also: the equivalence-key delimiter was a literal NUL byte, which made git classify the whole file as binary and hide its diff. Escaped. * fix(external-media): stop the dedupe discarding half-states (#1162) Third review round. Five findings, four applied. - is_hidden joins the feedback equivalence key. feedbackService lets a moderator-hidden row coexist with the guest's visible replacement and counts only the visible one, so ignoring it deleted the visible row as redundant. - admin marks merge instead of dropping. rating and color_label are written independently, so the same admin can have rated one tile and coloured the other; the loser now hands over any field the winner has no value for. - a survivor that loses the only completed scan is requeued. Otherwise the purge takes the sole embeddings and nothing re-queues it — the photo just silently stops having a face. - view_count and download_count are carried over. Those are real interactions recorded per row, and dropping them quietly lowered the engagement the admin grid shows. Not applied: repointing a category hero can in principle land on a survivor in another category. It needs the two duplicate rows to have been re-categorised apart after the racing import, and the result is a cosmetic hero mismatch that the admin category routes already guard on write. Not worth the extra branch in a data migration. * fix(external-media): invalidate the download zip when duplicates are removed (#1162) External review of the stable twin. Applies to both branches. The pre-built "download everything" archive still contained the duplicate rows the dedupe had just deleted, so guests kept receiving them until something else happened to invalidate it. Every ordinary photo-deletion path calls downloadZipService.invalidate for exactly this reason. The columns are cleared rather than the service being called: that service carries debounce timers and a regeneration queue, which is not something a migration should start. getZipInfo already treats a cleared record as a cache miss and rebuilds on the next request, so this is the durable half of what invalidate does. The stale object is left in storage for the same reason the duplicates' thumbnails are — a migration is the wrong place to reach into a backend that may be S3. --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul Nothaft
|
c305ad4146 |
feat(faces): make "not the same person" survive a re-scan (#1132) (#1145)
A separation — an explicit dismissal, or the implicit one a Split records — was stored as a pair of event_people.id. Those ids do not survive re-derivation: recluster() deletes every person, and a full re-scan replaces a photo's faces outright, so face ids die too. The only thing that survives both is the embedding, so the decision is keyed on the two centroids the pair had when the photographer separated them. It binds while both sides still look like the clusters that were separated, and lapses once they have drifted past recognition. The constraint is now honoured at assignment time as well as in consolidate(), which is what makes it hold across a re-scan rather than being reformed before any later pass could object. Six review rounds shaped the matching itself: each candidate must resolve to the OPPOSITE side rather than merely matching something (a split leaves two similar halves, and the loose test fragmented the person the split was not even about); assignment judges both sides at the ordinary match threshold, since a single face — or a cluster of one part-way through a recluster — cannot resemble a settled centroid; separations carry their own model_version; and the projections are hoisted out of the innermost loop, which took a 2000-photo scan from ~15s of dot products to 0.23s. Lifecycle closed three ways: purgePhotoFaces re-anchors each side onto the live cluster it still describes and drops rows that describe nothing left, deleteEventCascade and the permanent archive delete clear the table (which deliberately has no event FK), and a later manual merge drops the separations it reverses. All of it matched on vectors rather than ids, since a row that has outlived a recluster names people who no longer exist. Merged with admin privileges: the author cannot self-approve. |
||
|
Paul Nothaft
|
2c81888eaf |
fix(gallery): a guest's own hidden feedback is hidden from them too (#1150) (#1153)
Everything in the system treats a hidden row as absent — getPhotoFeedback drops it even for the guest's own feedback, and updatePhotoFeedbackStats does not count it. The per-viewer is_liked heart and my_color_label badge read the row without looking at is_hidden, so a like the photographer had hidden still showed as liked on a photo whose like_count was zero. Making those agree exposes why it had not been fixed: the duplicate check behind like/favorite toggling did not skip hidden rows either, so the now-empty heart, when clicked, found the hidden row and toggled it OFF — the click did nothing visible and the moderation was silently undone. Skipping hidden rows there makes the click create a fresh, visible row. Review found four more surfaces still treating a hidden row as present: the per-guest caps (an at-cap guest with one hidden met their own click with limit_reached), /my-feedback (which drives the Liked/Favorited/Rated chips in guest identity mode), getEventFeedbackSummary (disagreeing with the photo counters in the same response), and unhide (leaving two visible rows for one guest). The rating-clear and single-value delete scopes are visible-only now, so a follow-up mutation no longer destroys the admin's hidden record, and the unhide collapse is skipped when there is no stable identity to scope by — that fallback was 'guest_identifier IS NULL', i.e. other visitors' rows. Not taken: refusing to hide non-comment feedback, which the issue recommended. #839 and #1044 both ship hiding for reactions and colour labels with tests asserting a hidden one stops counting; only the admin UI's Hide button is comment-only. Merged with admin privileges: the author cannot self-approve. |
||
|
Paul Nothaft
|
00b20b2d72 |
fix(gallery): guest filters respect show_feedback_to_guests, and marks survive a mid-write clear (#1147)
Two follow-ups from the review of #1137. Filters were a second way to read hidden feedback. Every token on /photos is an OR of two halves: what THIS viewer marked, and what ANYONE marked. The response fields built from the second half — like_count, comment_count, color_label_count — are all gated on show_feedback_to_guests. The filter was not, so with the setting off a guest could still send ?filter=liked and get back exactly the photos other people liked, across all five tokens. Reachable by a direct API caller holding a gallery token; the frontend never sends filter to this endpoint. The half it left standing was also the wrong half. It read guest_identifier from the guest_id QUERY PARAMETER, which never matched anything — the frontend invents that string in localStorage and never sends it when submitting feedback, while submissions store generateGuestIdentifier(req). So gating the aggregate would have emptied these filters rather than narrowing them to 'mine', and accepting a caller-supplied identifier was a way back through the gate. Resolved from the request now, hidden rows excluded to match what the viewer can see. A mark whose row is cleared mid-write lost its value. #1137 fixed two calls both writing; this is one clearing while another sets. The clear empties the row, the row is deleted for being empty, and the setter's update matches nothing — the caller told 'no mark'. A zero-row update now reports itself and the caller re-reads, bounded at three passes, throwing rather than reporting a success that did not happen. Merged with admin privileges: the author cannot self-approve. |
||
|
Paul Nothaft
|
b581267031 |
fix(scripts): regenerate-thumbnails resolves external sources through ensureThumbnail (#1148) (#1151)
The CLI fallback carried the defect #1129 fixed in the admin route: it computed `storage/events/active/<photo.path>` and fs.access'd it, a location that does not exist for external or reference rows. Every one failed the check and was counted as an error, so on an external-media install the script was inert while reporting one error per photo. Resolution now goes through ensureThumbnail, which already branches on source_origin and owns the per-photo ext<id>_ output name — sharing it is what stops the script and the route drifting apart again. Also: videos skipped on every marker they can carry (fileWatcher writes type and mime_type but never media_type), responsive tiers backfilled alongside the canonical rendition, skip-vs-generate asked from isThumbnailValid rather than inferred from an unchanged path, tier failures counted rather than swallowed, and a nonzero exit when the backfill was incomplete. The script is now importable with the CLI behind a require.main guard; it previously ran on require and called process.exit, so it could not be tested at all — which is why this survived #1129. Merged with admin privileges: the author cannot self-approve. |
||
|
Paul Nothaft
|
3583c924da |
feat(faces): consolidate look-alike clusters after a scan, and suggest the rest (#1107)
consolidate() has existed since #1074 and described this exact symptom in its own comment, but its only caller was recluster() — i.e. when an admin pressed Re-group people. After a normal background scan the centroids converged and nobody looked, so a gallery settled with 14 people that should have been 8. It now runs when a scan drains. There is no scan-finished event to hook, so an idle worker asks whether the events it touched have actually drained — 'a worker went idle' is deliberately not treated as sufficient, because with concurrency above one the others may still be working. The uncertain band asks instead of acting: pairs between the assignment threshold and the stricter auto-merge one surface as accept/dismiss suggestions, with sticky dismissals. Nothing merges silently — a pass that merged anything reports it and points at Split. Review rounds hardened it against overruling explicit decisions: it no longer absorbs ignored clusters (mergePeople ORs is_ignored onto the survivor, which would have hidden a real person), no longer merges dismissed pairs, no longer undoes a manual Split (which now records a separation), and no longer runs after detection is switched off. The dismissal read fails closed, a failed pass is retried with backoff rather than lost or hot-looped, and the new table follows event_people out of exports and backups. Name autocomplete needs no endpoint — the people list already open is the source, and it is event-scoped on purpose. Known limitation, tracked in #1132: separations are keyed on person ids, so a full re-scan loses them. Reported by @BraynArts. |
||
|
Paul Nothaft
|
97d92f8428 |
fix(thumbnails): regenerate external photos instead of dropping their tiers (#1129)
POST /admin/thumbnails/regenerate resolved every source as storage/events/active/<photo.path> and fs.access'd it. External and reference rows are not there, so every one failed and was counted as an error — and because the tier deletion runs first, the endpoint dropped every ?w= tier and rebuilt nothing, leaving the library worse than before it ran. The UI reported success either way. Now routed through ensureThumbnail, which resolves both source kinds, uses the per-photo ext<id>_ output name, and writes thumbnail_path back itself. Review rounds also removed both destructive deletes in generateThumbnail: the pre-delete ran before sharp opened the source, so an unreadable source left the previous rendition gone and the database pointing at it — across a bulk run, the whole gallery. Neither delete was needed, since put stages to a temp file and renames atomically and is the last statement in the try. Videos are filtered out, and the superseded rendition is removed only when the storage key actually moved, compared through the same canonicalisation the backends apply so a legacy backslash path is not mistaken for a different object. Reported by @BraynArts, who also identified the fix. |
||
|
Paul Nothaft
|
f735d26422 |
fix(gallery): a missing thumbnail tier must not take the backend down (#1128)
The first load of a gallery whose ?w= tiers do not exist yet could exit the Node process — not 500 one tile, kill the backend. Two defects stacked. The reader: LocalFsStorage.get() returns a lazy fs.createReadStream, so an ENOENT arrives after the await returned and outside the route's try/catch. An unhandled 'error' event is a process-level throw. pipeStreamToResponse attaches the handler the routes were missing — 404 for a vanished source, connection destroyed if bytes are already on the wire, file headers cleared so the JSON error is not served as image/jpeg or cached as a broken tile for an hour. Applied to all nine streaming responses in gallery.js. The writer: ensureThumbnailAtWidth passed regenerate:true, whose first act is to DELETE the target — on a path only reached when the tier is absent. A grid fires one request per tile, so one request unlinked the file another had just published and handed to a reader. Without the flag the write is an atomic rename. Generation is now also deduped per tier key: 8 concurrent requests ran 5 Sharp passes before, 1 after. Reported with a full diagnosis by @BraynArts. |
||
|
Paul Nothaft
|
bbce3cd2a2 |
feat(faces): let the photographer choose which photo represents a person (#1119)
Phase 1 of #1096. Clustering picks the cover, and its idea of a good one and a human's do not always agree. A cluster whose avatar is turned away or softer than the rest stays that way in the guest-facing people strip too, and nothing in the UI could change it. A picker reachable from each person row, reusing the face list the split dialog already loads — same query, same grid, different action on a click. Making the choice actually stick took four changes --------------------------------------------------------------------------- event_people.cover_face_id has existed since migration 177 and the PATCH already accepted it, so the first version of this was frontend-only. It was also a no-op: - facePeopleService.listPeople SELECTED cover_face_id and then discarded it, recomputing the cover as the best-scoring VISIBLE face on every read. The picker saved, said so, and the avatar reverted immediately. It now prefers the stored pick whenever this audience can see it, and falls back to the score-ordered choice otherwise — so visibility scoping still wins, and a guest is never handed a crop of a photo they cannot open. - recomputeCentroid overwrote cover_face_id unconditionally. It runs on rescan and on photo replacement, so any reprocessing silently undid a deliberate choice. It now keeps the chosen face while it is still a member of the cluster. - The face list is cached per person, and split/merge move faces between people. Until now the only reader closed itself after acting, so nobody saw the stale copy; the picker is a second reader of the same key. - cover_face_id meant two things. assignFaces seeded it with whichever face opened the cluster and recomputeCentroid overwrote it with the highest scoring one, so an automatic guess was indistinguishable from a deliberate choice — and honouring it would have pinned every UNCURATED person to that guess, which is worse than the fallback it replaced (the fallback is computed per audience and skips photos a guest cannot open). Both writers are gone, migration 179 clears the stored guesses, and the column now means one thing. That also removes the need to defend the choice against rescans: nothing overwrites it, and a dangling id self-heals to the derived cover. Clearing existing values is safe rather than destructive: no install has ever been able to SET a cover, so every stored value is an automatic guess by construction. Also fixes a PostgreSQL-only 500 --------------------------------------------------------------------------- GET /admin/events/:id/people/:personId/faces joined `photos` but did not table-qualify its WHERE, and photo_faces and photos BOTH have an event_id: column reference "event_id" is ambiguous Postgres refuses it, so the endpoint 500s and the Split dialog — its only consumer until now — has been broken on every PostgreSQL install since the join was added. SQLite resolves the ambiguity silently, which is why the suite stayed green. Reproduced against a real Postgres before and after. The query is now a named builder the route calls and the test imports, rather than a copy: an earlier version of that test re-declared the query, so the route could regress to the bare form while the assertions kept passing. Merge and recluster preserve the choice as well. Both already carried labels and privacy flags across; the chosen cover is human state of the same kind, so it now rides along — through a merge when the target has none, and through a recluster by following its FACE into whichever cluster ends up holding it, rather than the majority-descendant rule the label uses. The picker and the endpoint disagree past 500 faces, so the picker now says when it is showing a capped list rather than presenting it as exhaustive. Frontend suite 178 passing, backend 23 across the touched suites, build clean, no new type errors. Mutation-checked twice: dropping the cover preference fails the new listPeople test while the visibility-scoping test still passes, and restoring the auto-seed in assignFaces fails it too. |
||
|
Paul NothaftandPaul Nothaft
|
887bdbe6e5 |
feat(gallery): responsive grid thumbnails (#1095) (#1109)
* feat(gallery): responsive grid thumbnails (#1095) The half of #1095 that #1099 deliberately left out. Grid tiles are ~175 CSS px at the mobile 2-column default — about 530 device px on a DPR-3 phone — so the 300px thumbnail is upscaled ~1.8x and faces visibly mush. Backend mirrors the preview tiers exactly: ?w= on the gallery thumbnail route, whitelisted to 300/600/900, cached by width in storage, never written to photos.thumbnail_path, and keyed by photo id for every source type — basenames are not unique across events and a tier is served from a cache hit without re-reading the source, which is how the preview tiers nearly leaked one gallery's photo into another. The tier is in the ETag, or a client holding the 300px file gets a 304 for its 600px request. Cleanup and regenerate invalidation are wired the same way. generateThumbnail now takes width/height overrides; it keeps the configured `fit`, because the grid renders with object-cover and tiers that were framed differently would visibly jump as the viewport changes. The srcset only advertises tiers the SOURCE can fill. Thumbnails are generated withoutEnlargement, so a 400px original asked for 900 comes back at 400 — advertising "900w" would have the browser pick that candidate and upscale it, which is the reported softness made worse. That exact trap is why this was held back from #1099; the photo's own dimensions are now the guard, measured on the SHORT edge because thumbnails are square and a 4000x600 panorama can still only fill a 600 tile. A source that clears only one tier gets no srcset at all rather than a single pointless candidate. Two things this surfaced, both worth knowing separately: `npx tsc --noEmit` type-checks NOTHING in this project — the root tsconfig is `files: []` with project references, so the real command is `tsc -b`, which is what build:check runs. Under tsc -b the repo has 43 files with pre-existing type errors; this branch adds none, and the one error in a file I touched (PeopleManagerModal:91) is on main already and unrelated to the line I changed. * fix(gallery): wire grid tiers into the component that actually renders The srcSet landed in PhotoGrid.tsx, which nothing imports — GalleryView renders PhotoGridWithLayouts, and every grid layout funnels its tile through the shared PhotoCard. The frontend half of #1095 shipped nothing. Moved to PhotoCard, and switched from srcSet to a single sized URL, the same shape PhotoLightbox already uses for preview tiers. AuthenticatedImage fetches its src with the gallery bearer token and renders the blob; an <img> carrying a w-descriptor srcSet ignores src entirely, so that fetch would have been discarded and the browser would have issued its own — unauthenticated, and resolved against the page origin rather than the configured API host. One URL keeps the auth path and halves the requests. The tier comes from the tile's measured width via the IntersectionObserver entry, read on the same render that reveals the image so nothing is fetched twice. Column counts differ per layout and shift again with thumbnailScale, so the breakpoint table is only a fallback. Also closes what the tier cache leaked or served stale: - ensureThumbnailAtWidth short-circuits videos. Their thumbnail is a poster frame, so the tier path handed the video file to Sharp — after downloading it in full on S3, uncached, once per request. - The ETag names the tier actually served, not the one requested. A fallback to the canonical thumbnail was caching a 300px image under a 900px key. - Tier height scales from the configured aspect ratio instead of forcing a square; with fit:'cover' a 300x200 canonical and a 600x600 tier are two different crops and the photo reframed between tiers. - The canonical short-circuit compares against the configured thumbnail_width, not the 300 default, so a 600px install stops generating duplicate tiers. - Tier invalidation on /admin/thumbnails/regenerate, above the local-file check that skips S3 and external rows. - Tier cleanup in replacePhoto and deleteEventCascade. Both derive keys from the photo row, so the rows have to be read before they change or vanish. Preview tiers had the same two holes and are swept alongside. The clamp no longer drops a tier when the source falls between them: a 400px short edge asked for 600 returns all 400 pixels, where clamping to 300 threw 100 of them away. Backend 18 tier tests, frontend 22. Full suites green: 293 backend across the touched areas, 185 frontend, build clean, no new type errors. * fix(gallery): measure the tile, and stop regenerating the w300 tier Follow-up to the review of #1095. Closes the three items left open there, plus a defect the previous commit introduced. **The w300 tier regenerated on every request.** Decoupling the canonical short-circuit from the hardcoded 300 left generateThumbnail still tagging against DEFAULT_THUMBNAIL_WIDTH. On an install with thumbnail_width=600 a w=300 request wrote `thumb_<name>` while the caller probed for `thumb_w300_<name>`: the cache never hit, so every request re-downloaded the original and ran Sharp, and the file it left behind was in no cleanup list. The tag now follows the configured width, and thumbnailTierKeys lists all three widths — which one is canonical is a setting, so excluding 300 stranded exactly the file a 600-configured install generates. **The tier is chosen from the tile's measured width.** The observer entry only exists for `lazy` cards, and Mosaic, Masonry and Timeline don't pass it — Mosaic is 1-up on mobile where Grid is 2-up, so they are the layouts a breakpoint guess gets most wrong. Measured in a layout effect and gated: the image is not rendered until the width is known, so AuthenticatedImage never mounts with a src it has to replace. Attaching the observer ref unconditionally instead refetches every tile, since React flushes passive effects before the sync re-render a layout effect triggers — removing the gate makes the new single-request test fail, which is how that was confirmed rather than assumed. **Gallery Premium has its own card** and never reached the shared one, so its tiles kept pulling the canonical thumbnail. MasonryPhotoAlbum already hands the laid-out width to the render prop, so it needed no measurement. **Event rename orphaned tiers.** The key embeds the basename, so the DB update is the point past which the old keys cannot be derived. Dropped inside the filename-changed branch, not the loop body: unconditional would fire four storage deletes per photo on every rename, 20k calls against S3 for a 5,000-photo event that merely had its slug adjusted. Preview tiers had the same hole and are swept alongside. Carousel is the seventh layout and deliberately gets no tiering: its filmstrip thumbs are 80 CSS px, under the canonical 300 even at DPR 3. Tests: first PhotoCard suite (6), backend tier suite 21. Both new behaviours mutation-checked — reverting the width tag, the render gate, the measurement, or the rename sweep each fails a test. Full suites green: 298 backend across the touched areas, 191 frontend, build clean, no new type or lint findings. * fix(gallery): mount masonry cards once, into a measured layout Found while capturing screenshots for this PR, by attributing every thumbnail request to a photo id rather than eyeballing the grid. Masonry columns mode starts at 3 columns and runs its greedy distribution off a hardcoded 300px estimate until the container has been measured. Cards mounted into that guess are torn down when it settles — photos move to a different parent column, so React unmounts them — and since #1095 each mount picks its tier from its own width, the two mounts request two DIFFERENT urls. Measured on a 1440px desktop, production build, 62 photos: before 45 photos fetched at canonical AND w600, 17 stuck on w600 107 requests after 62 photos, canonical only, 62 requests Mobile was already landing on one tier either way, so both mounts produced the same url and the second was a cache hit — which is why it looked clean and the desktop case did not. The fix is the gate the rows/justified mode in this same file already applies for the same reason (line 346): hold the cards back until containerWidth is known. Only columns mode was missing it. Grid and Justified take their column counts from CSS breakpoints, so they have no transient measured value to discard and are unaffected. Worth noting this was NOT visible on main: without tiering both mounts request the same url, so the browser cache absorbs the duplicate. Tiering is what turns a harmless remount into a second download — the regression is this PR's, which is why it is fixed here rather than deferred. Frontend suite 194 passed (3 new). Mutation-checked: removing the gate fails the mount-once and placeholder tests. --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
011f6ae7ec |
feat(gallery): sized preview tiers so phones stop pulling 1920px (#1095) (#1099)
* feat(gallery): sized preview tiers so phones stop pulling 1920px (#1095) A phone can display ~1170px at most, but the preview tier is a single 1920px JPEG with no size parameter — so every lightbox swipe ships roughly twice the bytes it can use, and the slide track preloads neighbours, which multiplies it. On the reporter's all-external install a null preview_url falls back to the untouched NAS original, which makes it worse again. Backend: ?w= on the gallery preview route, whitelisted to 640/1280/1920. A whitelist rather than a free-form width because every distinct value is a permanent rendition on disk — an open parameter is an invitation to fill the volume. Unrecognised or absent values fall through to the canonical 1920 preview, so old clients and hand-typed URLs behave exactly as today. Extra tiers are cache, not state: ensurePreviewImageAtWidth keys them by width, looks them up in storage and generates on miss, and never writes photos.preview_path. That column owns the canonical rendition, and threading a width through it would mean the last size anyone requested silently becomes "the" preview. Requesting 1920 resolves to the existing preview rather than a w1920 duplicate, so no install grows a second copy of every preview it already has. The tier is part of the ETag. Without it a client holding the 1920 rendition gets a 304 for its 640 request and renders the wrong size, which is this feature inverted. Frontend: the lightbox picks a tier from innerWidth x devicePixelRatio, capped at DPR 3 — uncapped, a DPR-10 device asks for 3900px and lands straight back on the desktop rendition. At the top tier the URL is left byte-identical so existing caches and ETags stay valid and desktop sees no change at all. saveData and a 2g/3g effectiveType drop one tier; both are Chromium-only, so they are a bonus rather than the mechanism. Grid thumbnails are NOT tiered here, deliberately. generateThumbnail resolves its width from admin settings rather than an argument, so tiering it is a separate change — and shipping a srcset whose candidates the server ignores would be worse than shipping none: the browser would take the "600w" candidate, receive the 300px image and upscale it, which is the reported softness made slightly worse. That half of #1095 lands separately. * fix(gallery): scope tier keys per photo, size by long edge, clean up tiers External review. Three findings against the tier work, one a cross-gallery leak. The tier cache key was the photo's BASENAME. Managed uploads keep camera basenames, so two events can each hold an IMG_0001.jpg — and a tier is served straight from a cache hit without re-reading the source, so the second gallery gets the first gallery's photo. Keys are now scoped by photo id for every source type. The RAW branch passed proc.outputBasename, which would have dropped that scoping again; it now passes the scoped name. Tier selection used viewport WIDTH, but ?w= bounds the LONG edge (fit:'inside'). On a 390x844 phone at DPR 3 a 2:3 portrait is bound by height and renders ~1755 device px, so width-only picked 1280 and made portraits softer than today; landscape on the same phone needs ~1170. It now computes the rendered long edge from the photo's own dimensions and falls back to the top tier — today's behaviour — when they are unknown. Tiers live outside photos.preview_path, so nothing else knew they existed: delete, bulk-delete and archive left them orphaned in previews/ forever, and regenerate-previews refreshed only the canonical rendition while phones kept the stale copy. previewTierKeys derives them from the same deterministic scheme and all four paths clean up. Deliberately outside the preview_path guard — a tier can exist when the canonical rendition never did, so keying cleanup off preview_path would strand precisely the photos only ever viewed on a phone. The existing tier tests encoded the old width-only semantics and were updated rather than kept; that is a behaviour change, not a test fix. --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
0b886ed942 |
fix(faces): defer on unreachable storage, and commit the import path first (#1097)
* fix(faces): defer on unreachable storage, and commit the import path first The two follow-ups left open by #1091, both consequences of external photos becoming scannable at all. **A dropped mount no longer burns the gallery.** ensurePreviewImage returns null for "this JPEG is corrupt" and "the NFS share is gone" alike, and faceProcessor marked both 'failed'. Nothing re-queues a failure automatically and the queue only ever claims 'pending', so a mount that blinked mid-scan cost the whole event a manual Re-scan — on external libraries, where network storage drops far more often than local disk, that is the common case rather than the corner one. faceProcessor now probes the containing DIRECTORY before failing, and throws TransientSourceError when it cannot be reached; faceQueue treats that exactly like SidecarUnavailableError — release to pending, back off, retry — with the warning rate-limited to one per five minutes, since an outage hits every photo in the event. The directory rather than the file is the whole point: a missing file inside a healthy directory is a broken photo and should still fail, and it still does. Anything that goes wrong deciding which case it is falls through to 'failed', because guessing 'transient' on an unknown condition would retry forever. **The import commits its path before inserting rows.** enqueueEvent accepts processing_status NULL (faceProcessor.js:243-246), which these inserts leave unset, so an admin hitting the toggle or Re-scan during a long import could queue partial rows while the event still resolved against the old directory — and burn them to 'failed'. Moving events.external_path ahead of the loop closes that, and fixes a pre-existing bug on the same line: an import that died at photo 500 of 1000 used to leave those 500 rows pointing into the new tree while the event still resolved against the old one, making every one of them unreadable. Safe to do first because the path is already validated above, and existing photos are unaffected — photo.source_origin takes precedence over event.source_mode in both resolvers and is NOT NULL defaulting to 'managed'. The #1090 test that asserted a missing source fails needed its setup corrected rather than its intent: it created no directory at all, which is now (correctly) a dropped mount. It now creates one, so it tests the case it always meant to — healthy storage, dead photo. Verified both fixes discriminate: removing the probe fails the defer test, and moving the event update back after the loop fails the ordering test. Face suite 59 passed across 8 suites; the full backend suite fails the same 10 pre-existing suites as unmodified main, no more. * fix(faces): stop a dead mount stalling the whole queue External review, and the first finding is one my own change created. Deferring by returning the row to 'pending' was a trap: claimNextPhoto orders by id ascending and the queue defaults to a single worker, so the same unreachable row becomes the oldest pending one after every backoff and the worker never reaches a higher id. One dead mount would have stalled face scanning for the entire install — unrelated events, fresh uploads, everything. Strictly worse than the permanent 'failed' this set out to replace. The row is now left parked in 'processing' with face_started_at intact. It is not claimable, so the worker moves straight on; the janitor that already exists returns it to 'pending' past STUCK_TIMEOUT_MS, which is the retry. No new column and no new timer. The sidecar branch still releases, because a down sidecar blocks every photo anyway — there is no other work to get on with. Second: probing existence was not enough. Unmounting an NFS or SMB share usually leaves the mountpoint behind as an ordinary empty directory, so fs.access succeeded on storage that was entirely gone and the photo was failed anyway — the exact case this was written for. An empty directory where the photo should live now counts as unreachable. The trade is deliberate and documented: a directory an admin genuinely emptied is retried rather than failed, which now costs one attempt per janitor sweep and nothing else. Third: a comment in adminExternalMedia claimed source_origin isolates existing photos from the early external_path update. That is true of managed rows and false of external ones — resolveExternalPath prefixes every external row with event.external_path, so importing folder B into an event referencing folder A rebases the A rows. Pre-existing rather than introduced here (the update always did this, just later), but the comment asserted otherwise, so it now says what actually happens and names the underlying single-base-path limitation. The deferral test initially passed against the blocking version too — database state alone cannot tell the fix from the bug. It now inspects the branch directly, the way the #596 contract tests do, and fails when releaseToPending is put back or the two branches are merged. * fix(faces): back off per event, and stop clobbering concurrent scans Round two of external review. Parking a row in 'processing' fixed the head-of-line block but not the cost: every janitor sweep handed the whole dead gallery back, and the worker walked all of it again — one stat per photo against storage that may be hard-mounted and slow to time out — before reaching any healthy event. Every one of those attempts also went through generatePreviewImage first, which logs an error per photo, so a down mount produced a recurring flood that the rate-limited warning did nothing about. So the backoff is now per EVENT and separate from the janitor: TransientSourceError carries the event id, the queue records a cooldown, and claimNextPhoto excludes those events while it lasts. The janitor keeps doing its own job, which is rescuing rows a crashed worker abandoned. Cooldown is in memory on purpose — a restart is usually what follows fixing a mount, so it should retry at once. Second: committing the event path before the loop means a toggle or Re-scan firing mid-import can now genuinely queue and finish some of those rows. The final bulk update was unconditional, so it dragged 'done' rows back to 'pending' for a duplicate sidecar scan and knocked 'processing' rows out from under the worker. It is now whereNull — only rows nothing has touched are ours to queue. Also corrected a comment of mine that had gone stale in the same file: it still described the enqueue as happening after the event path was written "below", which stopped being true when that update moved above the loop. * fix(faces): judge the mount, the path and the file separately Round three of external review. The probe was too coarse in both directions. It read any ENOENT on the photo's own directory as a mount-wide outage, so a deleted or renamed subfolder — individual/ gone while collages/ is healthy — deferred the entire event and starved every sibling folder, renewing the cooldown on each retry. It now judges the EVENT ROOT for that verdict: root missing, or present-but-empty, is an outage; anything below a populated root is a broken path and fails. And it read a listable directory as proof the photo was at fault, so EACCES on a reconnected share, EIO, or the classic NFS ESTALE handle were burnt as permanent failures. Only ENOENT now means genuinely gone; any other error opening the file defers. The event-wide backoff was also too broad. A reference event can hold managed uploads alongside imported external ones, and those live in local storage that is fine — excluding the whole event id left them unscanned for as long as external rows kept renewing the cooldown, which during a real outage is indefinitely. The exclusion is now scoped to external and reference rows. One of my own tests had modelled the unmount wrongly: it emptied the photo's subdirectory rather than the event root, which under the corrected logic is a populated mount with a missing folder — a failure, not an outage. It now empties the root, which is what an unmount actually leaves behind. Dropped the path require the first version of this probe needed; the event-root form does not. All three fixes mutation-checked: reverting each one fails the test written for it. Face suite 69 passed across 9 suites; full backend suite fails the same 10 pre-existing suites as main, no more. --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
576924fa57 |
fix(faces): scan external/reference photos instead of skipping them (#1090) (#1091)
* fix(faces): scan external/reference photos instead of skipping them (#1090) faceProcessor short-circuited every photo with source_origin 'external' or 'reference' straight to 'skipped', before the sidecar was ever contacted. On an external-media install that is the entire library — the reporter's gallery sat at 0/3230 with every row skipped and no error, and a rescan changed nothing. The guard was correct when written: resolvePhotoStorageKey returns null for anything outside managed storage, so ensurePreviewImage could not build a preview and there was nothing to send. #1078 removed that limitation one release earlier — ensurePreviewImage now reads externals straight off the mount via resolvePhotoFilePath and writes the preview into managed storage, so the key faceProcessor already fetches through getStorage() is readable like any other. The guard outlived its reason. Photos whose source is genuinely gone still return a null preview key and land in the existing 'failed' branch, which is the honest outcome: that is a broken photo, not an unsupported one. The blanket skip was absorbing those too. No migration or manual reset needed — enqueueEvent already re-queues rows with face_status in (NULL, 'failed', 'skipped'), so previously skipped photos get picked up on the next scan. * fix(faces): queue external imports for scanning (#1090) The other half of the same bug, found by external review — and my first counter-argument against it was wrong. Managed uploads are enqueued by photoProcessor, which writes face_status 'pending' once a photo is processed (photoProcessor.js:573, commented as "the only correct place to enqueue"). External media never goes through photoProcessor at all: adminExternalMedia inserts rows directly, leaving face_status NULL. faceQueue.claimNextPhoto only claims 'pending' (faceQueue.js:64), so an import into an already-enabled event produced nothing until someone pressed Re-scan. Lifting the skip guard alone made external photos scannable but still not scanned — which looks like a complete fix right up until you import a photo. Resolved once per import rather than per file, since it is a per-event setting and the loop can run to a thousand files, and guarded on both the global flag and the per-event toggle exactly as photoProcessor guards it, so installs without the feature still never write a face_status. A failure to read the setting logs and imports anyway — the photos are the point. No video guard: walkDir only collects jpg/jpeg/png/webp, so nothing faceProcessor would skip as video can arrive through this route. * fix(faces): enqueue imports only after the event path is written External review caught a race I introduced in the previous commit. Marking rows 'pending' as they were inserted published claimable work while events.external_path still held the old value — or none at all, on a first import, since the route only writes it after the entire thumbnail loop. The face worker polls continuously, so on any import long enough to matter (the loop is ~100-300ms per photo, and the reporter's library is 6500+) it would claim those rows, resolve them against the wrong directory and mark them permanently 'failed' — a state only an explicit Re-scan clears. That is strictly worse than the unscanned photos this set out to fix. Ids are now collected during the loop and marked pending in one pass after the event path is written, chunked at 500 because SQLite caps a statement at 999 bound parameters. The test now drives the real route instead of re-implementing its logic, and observes the mid-loop state from inside the per-photo thumbnail call — the only hook that can see the window the race lived in. Verified it discriminates: deleting the enqueue fails two tests, and moving it back onto the insert fails the ordering test specifically. * fix(faces): read the face setting after the import, not before Third external-review round. The setting was captured before a loop that runs for many minutes on a large library, so an admin who enabled detection during an import left every photo imported after that moment at NULL forever — the toggle endpoint only queues rows that already existed when it fired. Ids are now collected unconditionally and the setting is evaluated immediately before the queue update, off a freshly read event row. The guard is unchanged in substance: both the global flag and the per-event toggle, so installs without the feature still never write a face_status. Test flips the toggle from inside the mocked per-photo thumbnail call, which is the same mid-loop hook the ordering test uses. --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
b69dd134d0 |
feat(faces): People in this gallery — face recognition via an optional ML sidecar (#1074) (#1075)
* feat(ml): optional face-detection sidecar, opt-in and inert by default (#1074) First of four PRs for "People in this gallery". This one ships only the sidecar, its wiring and its CI — no schema, no backend code, no UI. Nothing in PicPeak calls it yet. picpeak-ml is a single FastAPI + onnxruntime container: three endpoints (/health, /info, /faces), no database, no volumes, no egress, no model download at runtime. Clustering, person identity and every privacy decision stay in the backend where the data already lives. Models are YuNet (detection) + FaceNet-512 (embedding), both MIT, both pinned by URL and SHA-256 and verified at build time. The licence analysis is in ml/LICENSES.md: the more accurate InsightFace weights are non-commercial-only and PicPeak's users are working photographers, so they are never baked into an image we publish. Two things worth review attention: - Alignment uses a least-squares similarity transform (Umeyama), NOT cv2.estimateAffinePartial2D. RANSAC and LMEDS exist to reject outliers among many correspondences; given five landmarks and no outliers they fit a three-point subset exactly and let the rest drift. Measured on a real off-frontal portrait: eyes and nose pinned to 0.11px, mouth corners 11.8px out on a 160px crop. Umeyama distributes it (max 6.5px, rms 5.1 vs 7.4). The failure mode is silent — a bad warp still yields 512 confident floats — so tests/test_pipeline.py pins it numerically. - FACENET_ONNX_URL has no default and the build fails loudly without it. deepface distributes FaceNet-512 as Keras .h5 only, so the ONNX is produced once by tools/convert_facenet.py and published as a release asset. Converting inside the build would drag TensorFlow through both architecture legs of every build to produce a byte-identical file. The CI jobs are gated on the FACENET_ONNX_URL repository variable and skip cleanly until it is set. Off by default, twice over: the sidecar is behind the `faces` compose profile, and the backend will gate on a `faces` feature flag that defaults to false. FACE_ML_URL defaults to http://picpeak-ml:8000 so the standard deployment needs no configuration — nothing dials that host while the flag is off, which is why a non-resolving default is harmless. Verified: 27 pytest tests green; YuNet loads and detects against a real portrait with its landmark order matching the alignment template index-for-index; both compose files validate and the faces profile is correctly excluded from a default `up`; workflow YAML parses and the job graph resolves. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(ml): pin the converter toolchain, verify parity, drop a false reproducibility claim (#1074) Ran the FaceNet-512 conversion for real and corrected what the previous commit assumed about it. The conversion works: 23,497,424 parameters, 89.6 MB ONNX, and the converted graph matches the Keras original to 2.086e-06 absolute / cosine 1.0000000000. That check is now part of the script rather than something I did once by hand — a subtly wrong graph still returns 512 plausible floats, so it refuses to leave the file on disk if parity fails. Also ran the full pipeline against both real models end to end. The embedding is L2-normalized to 1.000000, and the same face survives being re-rendered: half scale 0.973, double scale 0.984, JPEG q40 0.987, rotated 8 degrees 0.984, brightness +40 0.988. Scale invariance in particular is evidence the alignment warp is doing its job. Corrected claim: the conversion is NOT byte-reproducible. Two runs with the same pinned versions on the same machine gave different SHA-256s. The graphs are functionally identical — same 336 nodes, same 271 initializers, every weight matching to 0.000e+00 — but a few initializer names differ because tf2onnx's traced-op naming is not deterministic (Keras layer naming is deterministic; I checked). The previous commit message and README both claimed byte-identical output. They were wrong, and it matters: anyone re-running the conversion gets a different hash, and without this note that reads like tampering. The build-time SHA-256 pins one published artifact so its URL cannot start serving different bytes; validating a fresh conversion is the parity check's job. requirements-convert.txt now pins the exact set that produced the artifact, including transitive keras/protobuf/numpy, and documents that the converter needs Python 3.11 while the image runs 3.12. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * feat(faces): schema, queue, clustering and API for People in this gallery (#1074) Backend half of the feature. Migration 177, a face-detection queue, the clustering engine, the gallery and admin APIs, and the privacy wiring. No UI yet; nothing is reachable until the `faces` feature flag is on, which defaults to false. The flag is the gate, not FACE_ML_URL. That variable now has a working default (the compose service name), so its presence proves nothing about intent — if it were the gate, every install would poll a hostname that does not resolve. faceQueue re-checks the flag every tick, so turning it off stops the workers without a restart. Visibility scoping is the part worth reviewing closely. Face rows have no concept of photo visibility, but guests are restricted to photos.visibility='visible'. A raw count leaks how many hidden photos someone appears in, and an unscoped cover face renders a crop of a photo the guest may not open — with the best-scoring face being the likeliest pick, so it would happen often rather than rarely. facePeopleService recomputes both per request against the caller's own scope, and event_people.face_count_total is named to be conspicuous in a guest path. Six tests cover it, including the case where a person's photos are ALL hidden and they must vanish entirely. Face data is excluded from backups and .picpeak exports, per the decision in the thread: it is derived, so a restore re-scans rather than carrying biometrics between operators. Three separate mechanisms, because the engines cannot be filtered alike — EXCLUDED_TABLES for export, --exclude-table-data (not --exclude-table; the CREATE TABLE must survive or restore breaks on the first query) for Postgres, and DELETE + VACUUM on the temp copy for SQLite, which has no way to exclude a table from a whole-file .backup. The VACUUM is not cosmetic: without it the pages stay in the file and the claim is false on disk. Archiving now purges face data explicitly. photo_faces cascades off photos, but archive deletes neither the photo rows nor the event, so without this an archived gallery kept its biometrics indefinitely. Other decisions: clustering keeps names across a re-cluster by majority inheritance (without it, one button click silently discards every name the photographer typed); consolidation refuses to merge two people who were named differently; assignment never compares across model_version, since embeddings from two pipelines are not comparable; low-quality faces are stored but left unassigned so they show in "this photo contains" without spawning junk people. Migration is 177, not 174 — 174/175/176 landed on main while this branch was open. 29 tests green: 7 migration (idempotency, down(), cascade, and that installing it enqueues NOTHING), 11 clustering, 11 privacy/visibility. Lint clean; the pre-existing error counts in databaseBackup.js and server.js are unchanged. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * feat(faces): People strip, face filter and admin controls (#1074) Frontend half. Renders nothing anywhere unless the `faces` feature flag is on AND the photographer enabled detection for the gallery — the whole guest surface hangs off one boolean, event.people_enabled, which the server computes from the flag, the per-event toggle and the show-to-guests toggle together. Guest side: a People strip between the filter bar and the grid, circular crops from each person's cover face, an active-filter chip row, and a "Show all" bottom sheet. The face filter composes with category, search, media type and the liked/saved/rated filters in the same useMemo rather than replacing them, so "photos of Anna that I liked" works. Two people selected means AND by default — that is what picking a second face almost always asks for — with a toggle to OR that appears only once a second person is picked. Unnamed people show a photo count and never "Person 7". A number is honest about what the system knows; an invented name is not. There is a test asserting we don't do it. The strip renders nothing below two people, collapses to one line when dismissed (persisted per slug, so dismissing one gallery says nothing about the next), and appears mid-backfill with a progress line rather than blocking the gallery behind a spinner. Avatar crops are computed in ratios of the source dimensions so they survive whatever rendition the browser gets; without width/height they fall back to an uncropped thumbnail, since a wrongly-offset crop is worse than no crop. No new download endpoint: "download these N" rides the existing photoIds path, which already enforces access level and per-category permissions server-side. Adding a person_id selector would have been a second thing to authorize for no gain. Guest-facing copy never says "biometric" or "recognition" — those words describe our implementation, not the guest's experience. The sheet's footnote answers the first question every guest has (where does this go?) inline. The admin card, by contrast, is explicit: it states the controller obligation next to the toggle, and warns that scanning materializes the preview tier on galleries that never generated one, which is real CPU and disk an admin should know about before a 2,000-photo backfill. EN + DE translations. 140 frontend tests green (8 new), tsc and eslint clean. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(faces): measured match threshold, working build defaults, 89MB smaller image (#1074) Ran the Phase 0 spike that had been outstanding, published the model, and fixed what both turned up. THRESHOLD IS NOW MEASURED, NOT GUESSED. LFW's standard 1000-pair protocol run through this exact pipeline (YuNet -> Umeyama alignment -> FaceNet-512 ONNX), 100% detection on 2000 images: same person cosine 0.6958 +/- 0.1415 diff person cosine 0.0849 +/- 0.1674 separation 0.6109 peak accuracy 96.60% @ 0.405 So the pipeline separates people well — the thing I could not previously claim, since every earlier number was the same face re-rendered. Default moves 0.62 -> 0.50. The old value was a placeholder and a bad one: it gave 0% false merges but 22.4% false splits, i.e. roughly one in four same-person pairs failing to join, which fragments a gallery badly. 0.50 gives 1.0% false merge / 8.2% false split. Peak accuracy (0.405) is deliberately NOT chosen: for clustering the two errors do not cost the same. A false split is a duplicate row the photographer can merge away; a false merge puts a stranger into someone's "download my photos" — and until the Phase 2 merge/split UI ships, there is no way to undo one. So this sits on the conservative side of the optimum. The spike is committed as ml/tools/benchmark_threshold.py rather than thrown away, so "why 0.50?" has an answer in six months and a re-tune is one command. BUILD DEFAULTS. FACENET_ONNX_URL/_SHA256 now default to the published ml-models-v1 release asset, so `docker build ml/` and `docker compose --profile faces up` work with no arguments. Blanking either still fails loudly — a URL without a checksum is never acceptable, since the checksum is what makes the URL safe to trust. Found by running compose for real: it failed exactly as designed, which was correct behaviour and a bad out-of-box experience now that a canonical artifact exists. IMAGE SIZE. 389MB -> 300MB single-arch. `chown -R` after COPY rewrote every copied file into a fresh layer, duplicating the 90MB model for nothing; the user is now created before the copies and ownership set via COPY --chown. Also drops pip/setuptools from the runtime image. Measured RSS is 186MiB idle, and the container answers /faces end-to-end in well under the 80-150ms/photo the issue budgeted. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(faces): threshold 0.50 -> 0.60 from real clustering, theme-aware People strip (#1074) Both fixes come from running the feature on an actual gallery — 61 photos, 5 real identities — rather than reasoning about it. THRESHOLD. The LFW pairwise sweep in the previous commit said 0.50, and it was wrong. On a real gallery at 0.50, three of six visible clusters were contaminated: two different people merged into one strip entry, which is the exact failure that puts a stranger into someone's "download my photos". Pairwise error rates do not predict cluster purity. Greedy assignment compounds — one wrong face drags the centroid toward the midpoint between two identities, making the next wrong face likelier. A 1% pairwise false-merge rate is not a 1% chance of a clean gallery, and no amount of staring at an ROC curve would have shown that. Sweep against ground truth (5 identities): 0.50 -> 6 clusters, 3 contaminated 0.56 -> 6 clusters, 0 contaminated 0.60 -> 5 clusters, 0 contaminated <- exactly right 0.64 -> 5 clusters, 0 contaminated, fewer faces assigned 0.60 recovers the right number of people with no contamination; higher only loses coverage. Migration 177 carries the full reasoning so the next person to touch this knows why the obvious pairwise answer is the wrong one. THEME. The People strip hardcoded `text-neutral-800` for named people. On a dark gallery — which the screenshot immediately showed — that renders a named person's label almost invisibly, while UNNAMED people stayed legible. Exactly backwards. Labels, headings, the collapsed summary, the scan line and the filter chip row now read the gallery's own theme tokens (--color-text / --color-muted-text / --color-accent / --color-surface-border) like the rest of the gallery surface. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(faces): keep the mobile filter row inside the viewport (#1074) At 390px the photo count and Clear link were pushed against the right edge by ml-auto and clipped. Only apply it from the sm breakpoint up, where there is room; below that they flow after the chips. Found by screenshotting the real thing on an iPhone-sized viewport. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * feat(faces): complete Phase 1, add People management and auto-categories (#1074) Closes the two Phase 1 gaps, then builds Phase 2 and Phase 3. PHASE 1 GAPS. "Download these N" was specified, described as done in an earlier summary, and never actually built — I had verified the backend needed no new endpoint and let that stand as if the button existed. It now hands the filtered photo ids to the same path as a manual selection, so the server re-applies access level and per-category permissions on the way through. Photos in a downloads-disabled category are excluded client-side too, so the number on the button is the number the guest receives. Hidden entirely when downloads are off for the gallery. Lightbox person chips ("In this photo: Anna") are the second way into the face filter — a guest looking at a photo of themselves can act on it without scrolling back to the strip. Tapping one closes the lightbox and filters the grid behind it. PHASE 2. A People management modal over the endpoints that already existed and were already tested: rename inline, merge (multi-select, first pick is the target so the name a photographer typed survives), split via a face picker, hide, ignore. This matters more than it sounds — clustering deliberately errs toward splitting because a wrong merge puts a stranger into someone's download, and that trade only works if merging is easy. PHASE 3. Rule engine over face_count plus face-area ratio: 0 -> Details, 1 large -> Portraits, 2-5 -> Small groups, >5 -> Groups. The area ratio is what separates "a portrait of someone" from "someone is in this landscape". Three guarantees, all tested: it only ever fills an EMPTY category (enforced in the query AND re-checked in the UPDATE, so a photographer setting one mid-run still wins), everything it touches is marked auto_categorized so undo is exact, and it is a no-op unless separately enabled. Migration 178 adds the column — separate from 177, which has already run wherever this branch is deployed. Verified on the real gallery: 61 photos -> 48 portraits + 13 small groups, undo cleared exactly 61 and left the manual ones alone. Merge moved faces and removed the source. Both confirmed against the database, not just the UI. TWO BUGS THE BROWSER CAUGHT, both invisible to tsc: - The lightbox destructure never landed — my patch targeted a line that has a default value, matched nothing, and failed silently. `people` resolved to something else entirely and the chips would never have rendered. eslint's "outer scope value" warning is what surfaced it. - Admin face thumbnails 403'd because <AuthenticatedImage> attaches whatever gallery token is in session storage; an admin who has also opened one of their own galleries sends a type:"gallery" bearer to an admin route. Admin routes authenticate from the httpOnly cookie, which a plain same-origin <img> sends by itself. Worth noting AdminPhotoGrid has the same latent shape; not touched here. Also: the admin card now reports "N people (M shown to guests)" when those differ, so the settings page and the gallery stop disagreeing without explanation. 45 backend tests (8 new) and 140 frontend tests green; tsc and eslint clean. EN + DE for every new string. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * perf(faces): batch migration DDL and drop the face stack from server.js import (#1074) CI's backend job timed out at 10 minutes on the first run of this branch. Nothing failed — 132 of 182 suites passed and the wall clock ran out. Main does the same 182 in 124s, and where main has 12 suites slow enough for jest to print a duration, this branch had 77. Two changes, both worth making regardless of how much of the gap they close: - Migration 177 added its columns one ALTER TABLE at a time (four on photos, three on events, plus a separate index statement) and seeded settings with a SELECT and an INSERT per key. It now uses one alterTable per table and one SELECT plus one bulk INSERT. 178 folds its index into the same statement as its column. That chain replays in ~90 suites, so statement count there is multiplied by 90. - server.js required faceQueue at module scope, which pulls in axios and — through imageProcessor — sharp. Every supertest suite that imports server.js was paying for a module graph it never uses. Now required inside the startup block, next to the call that needs it. Honest about the evidence: locally the migration delta measures at zero (1.15s vs 1.13s for the same suite, three runs each), so batching alone does not explain an eight-minute regression. A fast local disk and many cores mask per-statement and per-import costs that a two-core runner with a shared disk does not. These are the two real costs this branch added to a path that runs in almost every suite; whether they are sufficient is a question for CI, not for another round of local speculation. 37 face tests still green after the change. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * i18n(faces): complete EN and DE coverage for the face feature (#1074) The admin card and the Features toggle were rendering entirely from inline English `defaultValue` fallbacks — 22 keys existed in no locale file at all, so a German admin saw an English consent notice, English toggles and English buttons. The gallery side was already translated; the admin side was not, and nothing in the toolchain flags this because a `defaultValue` always renders something. Adds the missing `admin.faces.*` (19), `settings.features.faces.*` (2) and shared `common.clear/saved/saveFailed` in both languages. Existing keys are left alone (setdefault, not overwrite), so the shared `common` strings other features rely on are untouched. Committed the audit as frontend/scripts/i18n-faces-audit.py rather than throwing it away: it extracts every t() key the face components actually use and diffs it against each locale, and it also reports German values that are byte-identical to English, which is the usual shape of an untranslated copy-paste. Currently: 69 keys in use, EN complete, DE complete, no identical pairs. Verified in the browser, not just in the JSON — the German card reads "61 / 61 Fotos durchsucht · 16 Personen (5 für Gäste sichtbar)" end to end. Also checked the components for hardcoded user-facing text (JSX nodes, title/aria-label/placeholder attributes) outside t(); there is none. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(faces): 13 defects from external review — coordinates, counts, erasure, races (#1074) Codex reviewed the branch against main. Thirteen findings, nine P1. I checked every one against the code and could not dismiss a single one as a false positive, so all thirteen are fixed here. THE WORST ONE: bounding boxes were stored in the wrong coordinate system. The sidecar reports coordinates in the space of the image it was HANDED — which is the ≤1920px preview, not the original — while every consumer compares them against photos.width/height, the original dimensions. A 6000px photo therefore produced boxes ~3x too small and areas ~9x too small: avatar crops landed in the wrong place and the Portraits rule could never fire. It is invisible on any photo already under 1920px, which is exactly why the demo gallery and every screenshot looked correct. Now scaled once in faceProcessor so everything downstream can assume original-image coordinates. ERASURE. The FK cascade on photo_faces is decorative on SQLite: PicPeak never enables `PRAGMA foreign_keys`, so deleting a photo left its embeddings behind. I first enabled the pragma globally and reverted it — six unrelated suites immediately failed on pre-existing dangling references, and switching it on would start rejecting inserts on every existing install. That is a real change worth making, but it is its own PR, not a rider on this one. Instead deletion purges explicitly: purgePhotoFaces in the photo paths (single, bulk, service) and photo_faces/event_people in deleteEventCascade. Tests assert this with the pragma explicitly OFF, so they can only pass if the code does the work. COUNTS. A re-scan deleted the old face rows without undoing their contribution to event_people, so counts inflated on every re-scan and ghost people survived. Now the affected people are recomputed before the replacements are assigned. My own "must not double its faces" test only checked photo_faces rows, which is why it passed throughout. RACES. A worker that finished after an admin purged the event committed its rows anyway — erasure reported success and the data reappeared. The commit is now conditional on the row still being 'processing'. And assignFaces is read-modify-write over an event's people, so two workers lost each other's updates; it is now serialised per event with an in-process mutex plus a Postgres advisory lock for the multi-pod case the queue advertises. METADATA LOSS. Merging discarded the source's name and suppression flags, so a merge could erase a typed name or un-hide someone. Reclustering remembered only people with a label, so an unnamed-but-hidden bystander came back guest-visible after one "Re-group people" — and suppression now propagates to every descendant cluster, not just the majority one. Also: export reset face_status so a restored gallery re-scans instead of claiming to be scanned forever; manual category edits clear auto_categorized so "undo automatic" cannot delete a photographer's own choice; external photos are skipped rather than failed (resolvePhotoStorageKey returns null for them by design); the gallery refetches photo memberships as a scan progresses so filtering is not stale; a failed VACUUM now fails the backup rather than publishing one that may retain biometric pages; and the ML Dockerfile's `|| true` is scoped to the uninstall — as written it was `(install && uninstall) || true`, so a failed dependency install produced a green layer and an image with no onnxruntime. Four new regression tests. Full backend suite failure set verified identical to origin/main; frontend 140 green; tsc and eslint clean. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(faces): 12 more defects from review round 2 — cross-event purge, leaks, lifecycle (#1074) Second Codex round on the same diff, now including round 1's fixes. Twelve findings, seven P1. Again none were false positives. SECURITY, AND MINE FROM ROUND 1: the bulk-delete face purge iterated the raw `photoIds` from the request instead of the event-scoped `photos` rows the handler had already validated. purgePhotoFaces has no event scope of its own, so an editor could pass another gallery's photo id and delete its face data — even though the photo deletion right below it was correctly scoped. Fixing one thing and introducing another is exactly why the second round was worth running. ANOTHER VISIBILITY LEAK, same class as the one round 1 fixed: /people returns scan progress, and getScanStatus counted every photo with a face_status — including hidden ones. Guests could read the hidden-photo count off the progress bar while the people list and covers beside it were properly scoped. Now scoped by the same predicate, with the caller passing its audience. RECLUSTER, ROUND 1'S FIX WAS INCOMPLETE. I made suppression follow every descendant but still copied the flags from the majority ANCESTOR. When reclustering merges a visible named person with a hidden one, the majority ancestor is often the visible one — republishing the hidden person's photos. Suppression is now OR-ed across every ancestor contributing faces. The name also now goes to the genuine largest descendant; the previous code took whichever cluster came first in map order, which the comment already claimed it did not. LIFECYCLE. Face data is excluded from backups and exports, but photos. face_status came across intact, so a restored install claimed every photo was scanned while holding no faces — and the worker only claims 'pending', so it stayed that way forever. Now: the SQLite backup requeues in the dump, restore requeues after the pool reinit (the Postgres path cannot rewrite rows inside pg_dump), the portable importer purges LOCAL face tables (they were excluded from the replace list, so another instance's embeddings survived an import with FK checks suspended) and requeues, and archiving disables detection so a restored archive is honestly off rather than enabled-and-empty. WRITE PATHS. Only processPhoto enqueued. The synchronous upload path (chunked-upload completion, watch-folder) left photos unscanned, and replacePhoto kept the OLD image's faces on a row now pointing at a different picture — stale identities shown on the new photo. FRONTEND. PeopleSheet and the admin manager rendered centred thumbnails and ignored the bbox, so on group photos the avatar showed whoever stood in the middle and two people from one photo were indistinguishable — in the manager whose entire job is telling faces apart. The crop maths is now one shared helper (faceCrop.ts) so the three surfaces cannot drift again. Full-page layouts (gallery-premium, gallery-story) render their own lightbox and never received the people props. Backend failure set verified identical to origin/main; frontend 140 green; tsc and eslint clean. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(faces): round 3 — five of round 2's fixes were wrong or no-ops (#1074) Third and final Codex round. Eight findings, four P1 — and the important part is that FIVE of them are defects in round 2's fixes, not in the original code. - The sync-upload enqueue I added was a silent no-op. It queried through `trx` after the transaction had already been committed, which throws "Transaction query already complete" straight into the catch I had wrapped it in. Chunked uploads and watch-folder imports were still never scanned, and the code read as though they were. Uses `db` now. - The post-restore requeue ran BEFORE the files were restored, in both the portable importer and the native restore. The face worker is live during a restore, so it could claim those rows and scan the previous instance's files, or fail them for originals not yet on disk — with nothing to requeue them afterwards. Both now run after file restoration; the native one is extracted into requeueFaceScans() and called from the full and database-only paths. - The admin face crop mixed coordinate spaces: an original-pixel bbox scaled against the THUMBNAIL's natural size. The API now returns the source dimensions alongside the box, so there is one space to reason about. - Forwarding people props through layoutProps did not make them work — the full-page layouts never destructured them. GalleryStoryLayout now threads them to its own lightbox. Genuinely new findings, all in the same class as ones already fixed: - releaseToPending updated unconditionally, so a photo purged while its sidecar request was in flight came back as 'pending' and was rescanned — biometric rows reappearing after the purge reported success. Round 2 fixed exactly this on the COMMIT path and I did not carry it to the retry path. Now guarded on 'processing'. - purgePhotoFaces left face_status alone, so a worker mid-scan still satisfied its commit guard and could write fresh faces into a photo being deleted — orphans, since the FK cascade is inert on SQLite. It now clears the claim as part of the purge. - Phase 3 was unreachable: the migration seeds face_auto_categorize_enabled false and nothing could ever write it, so the rule engine and its undo endpoint returned "disabled" in every real flow. Added GET/PUT and a toggle on the admin card, EN + DE. NOT fixed, deliberately: GalleryPremiumLayout uses yet-another-react-lightbox rather than the shared PhotoLightbox, so person chips there are a real port rather than a prop forward. Recorded as open rather than bodged. Backend failure set identical to origin/main; 41 face tests and 140 frontend tests green; i18n audit reports EN and DE complete at 71 keys. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * feat(faces): block face recognition on the all-in-one image (#1074, #1042) The single-container image cannot run this feature, so it is refused there rather than left to degrade. WHY, since the reason is not obvious from the code: the AIO image runs the backend, the frontend, SQLite and every background worker inside one container aimed at "one photographer plus guests browsing". It has no Redis, SQLite gives it a single writer, and it contains no ML sidecar to talk to. Face detection would add a second image-processing pipeline competing with Sharp for the same CPU and memory. That failure is not loud — the install just becomes slow and looks broken, which is the worst possible shape for a deployment whose whole promise is one container and no decisions. Gated on an explicit PICPEAK_SINGLE_CONTAINER marker, NOT inferred from SERVE_FRONTEND or a SQLite path: plenty of legitimate multi-container setups serve the frontend from the backend or run SQLite, and none of them should lose the feature by accident. Three layers, because the first is the only one that enforces: - faceSettings.isFeatureEnabled() returns false before consulting the flag, so a database restored from a full deployment with `faces` enabled still cannot switch it on here. - The feature-flag API forces `faces: false` in both directions, so the admin UI reflects reality instead of offering a switch that refuses to stay on. - The Features tab renders the card disabled with a plain-language reason, read from a new `single_container` field on /admin/system/version (an endpoint the admin UI already calls). Documented in ml/README.md and .env.example. Three tests pin the behaviour, including that the marker only accepts explicit truthy values. NOTE FOR PR #1068: this expects `Dockerfile.aio` to set `ENV PICPEAK_SINGLE_CONTAINER=true`. That one line lives on that branch and is not in this commit — until it lands, an AIO build would still offer the feature. Worth adding alongside the `Limits` section of docs/single-container.md. 44 face tests green; EN + DE complete at 72 keys. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * test(faces): pin the bbox coordinate space with a real scale factor (#1074) The coordinate-space bug — boxes stored in preview space while every consumer reads them as original-image pixels — had no test, and could not have been caught by the ones that existed: every photo in the demo gallery is 750px, so the scale factor was always exactly 1.0 and the correction never executed. Verified by hand first, on a real 4000x3000 upload with the face placed off-centre so a wrong crop would be unmistakable. Before the fix the stored box was 1493,204 (preview space, face actually at x≈2850-3618); after, 3110,426 — a factor of 2.083, exactly 4000/1920, landing inside the face. The admin crop then resolved to left=-395px/top=-46px on a 64px window, which is the face centred. That verification is now a test rather than a memory. Three cases: a 4000px photo must scale by 4000/1920, a 1920px photo must NOT change (the case that hid the bug), and a row with no width must fall back to unscaled rather than storing NaN. Note for anyone extending these: jest hoists mock factories above the file, so anything they close over has to be `mock`-prefixed. Getting that wrong fails at transform time with a message that does not name the variable. 47 face tests green. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
LucaandPaul Nothaft
|
8809564aad |
feat(backup): open sqlite → pg .picpeak restore as the supported upgrade direction (#1041) (#1043)
Reshaped onto main after #1039 landed the coercion engine (typedColumnsFor / epochToIso / coerceForTargetEngine) — this PR is now only the policy delta on top of it: - validateManifest: replace the CLI-only allowEngineSwitch opt-in with a direction rule — sqlite → pg allowed (upload UI and CLI alike), pg → sqlite refused with a message naming the supported direction - importFromPicpeak: derive crossEngine from the manifest's engine (absent field = target engine, the exact pre-change behavior), log it, return it; route passes it through - scripts/migrate-sqlite-to-postgres.js: rely on the shared gate, drop the flag - restore card: direction stated in the intro, cross-engine notice after a converting restore; both strings in en.json + de.json; removed the orphaned settings.backup.picpeak locale node (unreferenced, stale copy) - picpeakCrossEngine.test.js: direction policy, epochToIso (ms, seconds, numeric strings), coerceForTargetEngine units, plus PICPEAK_PG_TEST_URL-gated real-Postgres stored-value assertions Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul Nothaft
|
8e3573788b |
feat(downloads): per-gallery download resolutions (#858) (#1022)
Clients who need smaller files no longer make the photographer re-export. Two capabilities, both off by default. STANDARD RESOLUTION — the size a gallery hands out for every ordinary download (single, selected, download-all). Global default in Settings, overridable per gallery with the NULL=inherit tri-state. The pre-built download-all zip is built AT the standard resolution, so changing it invalidates those archives, including a fan-out to inheriting galleries. RESOLUTION PICKER — opt-in modal letting guests choose a different size. Custom archives are built as a DB-backed job the client polls, never cached. The picker never offers a size above the standard, and Original reappears only when the admin explicitly allows it. Resize is fit:'inside' + withoutEnlargement — aspect preserved, never upscaled — applied before the watermark, since the mark is sized relative to its input. Three rounds of external review hardened this: job archives are bound to the requester's visibility scope and re-validated at delivery, the streamed download-all path applies the cap, queue admission is bounded, and rejected resolutions no longer inflate download stats. Closes #858. |
||
|
Paul Nothaft
|
75bfad2b6a |
fix(slideshow): stop "no crop" fit letterboxing a pre-cropped frame (#1015) (#1018)
The slideshow resolved its image as preview_url || hero_url || url. preview_url is only emitted when lightbox_preview_enabled is on (default false), so a default install fell through to hero_url — the 1920x1080 fit:'cover' centre crop built for gallery header banners. object-fit: contain then letterboxed an already-cropped 16:9 frame, so portrait photos lost their top and bottom and 'Black Bars (No crop)' looked inert. Emits slideshow_url (same aspect-preserved preview tier) unconditionally for image photos; the show prefers it and never falls back to hero_url. preview_url stays gated so the lightbox opt-in is unchanged. Fixes #1015. |
||
|
Paul Nothaft
|
a607cea110 |
fix(auth): issuer-tag the oversize SSO logout marker (#798) (#1010)
Phase 3 validated a stored ID token hint against the currently configured issuer, but the oversize path never got that check: an ID token above the 3.9KB cookie limit was stored as the bare string 'sso', which collapsed to an undefined hint at logout and skipped validation entirely. Changing the issuer while such a session was live bounced the user to the new IdP on logout. Stores sso.<base64url(issuer)> instead and moves all marker interpretation into buildEndSessionUrl: raw ID token -> iss/aud-validated hint, issuer-tagged marker -> round-trip without a hint, anything else -> no round-trip. Every branch fails closed. Refs #798. |