Review remediation across backend, frontend, worker, and infra - #1
Merged
Merged
Conversation
Broad correctness, performance, and safety pass over the whole stack, plus the CI that would have caught most of it. Worker - recluster: cluster_selection_method was "epi", which HDBSCAN rejects inside fit() — the recluster job raised on every run. Faces attached to a *named* person are now marked manually_assigned and excluded from reassignment, so reclustering no longer destroys named people. - api: a parameterised route permanently shadowed a static one. - Heavy ML imports (torch, insightface) moved out of the modules under test into worker/stages.py and worker/video.py, and behind a lazy import in recluster._fit, so tests and typecheck run without the multi-gigabyte wheels. - Metadata: capture time is now resolved to a true UTC instant plus the camera's local wall clock and UTC offset, rather than storing local time labelled as UTC. Video rotation is read and applied. Backend (migration 0007_review_fixes) - Timeline had no index matching ORDER BY taken_at DESC NULLS LAST, so every listing degraded to a seq scan plus full sort. Added composite DESC indexes and dropped the ones they subsume. - Added missing indexes for duplicate-name checks, ordered collection reads, person media, share links by collection, and location facets. - taken_at_local / taken_at_offset_min / video_rotation columns so day and month buckets no longer shift with the viewer's timezone. - processing_at plus a partial index for a stalled-job reaper. - collections.person_id was ON DELETE SET NULL, which left a deleted person's auto-generated album and its public share links reachable. Now ON DELETE CASCADE. - pgvector and tsvector columns declared as Unsupported(...) in schema.prisma so a future migrate cannot silently drop them. Frontend - Shared query-key registry, useAppMutation, and error boundaries. - Date grouping derives buckets from the capture-local timestamp. Infra - backend.hcl and terraform.tfvars are now gitignored; Terraform state holds the IAM secret in plaintext. Added .example files for both. CI - New GitHub Actions workflow gating all four stacks. Every job fails the build; nothing is advisory. Includes a secret scan asserting no env or state files are tracked. - First tests in the repo: backend crypto and constants, frontend groupByDate and query keys, worker metadata, video, recluster, and a pipeline contract test.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
structlog.get_logger is annotated as returning Any, so under strict mode the type: ignore[return-value] was both unused and covering the wrong error code (no-any-return). Replaced with an explicit cast. Caught by the new CI worker job on its first run.
collectMediaS3Keys named the original, thumbnail, streaming and web keys, so two classes of object were orphaned in the bucket on every delete: referenced by nothing, and never cleaned up. - Face crops were only ever deleted when a *person* was deleted, so deleting a photo stranded every crop it had generated. - The responsive thumbnail ladder (`<uuid>@200w.webp` and friends) is stored at keys derived from thumbnailKey and recorded in no column — the client reconstructs them, so deletion has to reconstruct them too. Over-listing is safe: S3 DeleteObjects treats a missing key as success, so naming a variant that was never generated costs nothing. THUMBNAIL_WIDTHS now has a third copy (worker, frontend, backend). The backend needs its own because it is the only place that has to *name* these objects.
RayHCai
force-pushed
the
review-remediation
branch
from
August 13, 2026 03:50
fe17f02 to
d3b7bce
Compare
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Correctness, performance, and safety pass over the whole stack, plus the CI that would have caught most of it.
151 files, +16,654 / −8,955.
Crash fixes
Two defects that failed on every invocation:
recluster:cluster_selection_methodwas"epi". HDBSCAN only accepts"eom"or"leaf"and validates insidefit(), so the recluster job raised every run.worker/api.py: a parameterised route permanently shadowed a static one, making the static endpoint unreachable.Data-loss fixes
manually_assignedand excluded from reassignment.collections.person_idwasON DELETE SET NULL, so a deleted person's auto-generated album and its public share links stayed reachable. NowON DELETE CASCADE.Performance (migration
0007_review_fixes)The only index on the timeline sort key was ascending, which Postgres can walk backwards only as
DESC NULLS FIRST— so it could not satisfyORDER BY taken_at DESC NULLS LAST, created_at DESCand every listing degraded to a seq scan plus a full sort. Added composite DESC indexes and dropped the ones they subsume.Also added missing indexes for duplicate-name checks on upload, ordered collection reads, person media, share links by collection, and location facets.
Timezone modelling
taken_atstored the camera's local wall clock while labelling it UTC, so photos migrated between days depending on the viewer's timezone. Capture time now resolves to a true UTC instant (taken_at) plus the camera's local wall clock (taken_at_local) and offset (taken_at_offset_min); day/month buckets derive from the local value. Video rotation is now read and applied.The migration copies existing
taken_atintotaken_at_local, which preserves current bucketing — the stored value already is local wall clock. A metadata backfill will recompute both properly from EXIF.Secrets
infra/backend.hclandinfra/terraform.tfvarsare now gitignored — Terraform state holds the IAM secret in plaintext..examplefiles added for both. Separately, the shipped.env.exampledocumented an emptyWORKER_SECRET, which disabled auth across the entire/internalsurface; both secrets are now documented as required with minimum lengths.CI
There was no CI of any kind, which is the direct cause of both crash-on-every-run defects shipping, plus a declared ruff/mypy gate that had never executed. New workflow gates all four stacks — backend, frontend, worker, infra — and every job fails the build; nothing is advisory. A secret-scan job asserts no env or state files are tracked.
To keep the worker job to a couple of minutes rather than pulling multiple gigabytes of ML wheels, heavy imports (
torch,insightface) moved out of the modules under test intoworker/stages.pyandworker/video.py, and behind a lazy import inrecluster._fit.These are the first tests in the repo: backend crypto and constants, frontend
groupByDateand query keys, worker metadata, video, recluster, and a pipeline contract test.Frontend
Shared query-key registry,
useAppMutation, and error boundaries (app/error.tsx,ErrorBoundary.tsx).Notes for review
backend/package-lock.json(+5,607) is likely unwanted — this PR deletesfrontend/package-lock.jsonin favour of yarn, and the new CI runsyarn install --frozen-lockfileagainstbackend/yarn.lock. It looks like a straynpm installartifact and can be dropped. It was included because the request was to add all changes.Unsupported(...);prisma migrate diffshould report no drift.