feat(photos): firearm photo management with reusable upload/storage (#9) - #62
Merged
unclesp1d3r merged 18 commits intoJul 11, 2026
Conversation
Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…#9) Backend-agnostic StorageService interface (save/read/delete/generateKey) with a local-filesystem adapter behind a mounted uploads volume; path-traversal-safe server-generated keys and a derivative-key convention. U1. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Adds the firearm_photo child table (firearm-scoped, no own owner/grants), migration 0014, and a makeFirearmPhoto test factory. U2. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Validate MIME/size/batch, strip EXIF/XMP/IPTC location metadata by re-encoding, enforce a decoded-pixel cap, and generate thumb/preview derivatives. Adds sharp as a direct dependency and unblocks its install script. U3. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Owner/grant-scoped CRUD resolved through the parent firearm (child-record pattern): multi-file create with per-file results, primary uniqueness, delete with blob cleanup + primary auto-promotion, reorder, caption, quota, and a batched primary-thumbnail lookup. U4. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Add an opt-in pre-delete hook to the shared authorizeAndDeleteParent helper, wired only for firearm delete, that best-effort removes photo blobs; magazine and ammo delete are unaffected. orphanSweep reclaims unreferenced blobs, scanning the storage singleton's actual root. U5. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…ute (#9) Server Actions for upload (rate-limited), delete, set-primary, reorder, caption; an authenticated Route Handler streams photo variants with authz via the parent firearm and a pinned Content-Type + nosniff. U6. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Upload (multi-file, per-file result, loading state), thumbnail gallery, set primary, keyboard-operable reorder, caption edit/remove, delete, and an empty state. Photos serve via the authenticated /thumb + /preview variants. U7. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Compact primary thumbnail per row via a single batched lookup (no N+1), with a fixed-footprint placeholder when absent so the dense table keeps its rhythm. U8. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…tchers (#9) The e2e test server did not set UPLOAD_DIR, so served-app uploads failed; set an ephemeral per-run dir. Also make the gallery's primary-badge assertions exact so they don't match the 'Set primary' button's substring. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…ath (#9) Extract shared deletePhotoBlobs + photoVariantUrl helpers, run the three sharp re-encodes and orphan-sweep deletes concurrently, and select only the columns the serving path uses. Behavior-preserving; simplify pass over U1-U8. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Reject non-raster sharp formats before rasterization (an SVG labeled image/png would otherwise reach librsvg and fetch external refs — SSRF); cap reorderPhotos length like other bulk ops; and delete a photo's blobs after the delete transaction commits so a rollback can't strand a live row without its blob. From code review of U1-U8. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
10 tasks
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds firearm photo storage, processing, database persistence, authorization, authenticated serving, gallery management, list thumbnails, deletion cleanup, and automated verification. ChangesFirearm photo management
Sequence Diagram(s)sequenceDiagram
participant FirearmPhotos
participant uploadPhotosAction
participant createPhotos
participant PhotoServingRoute
FirearmPhotos->>uploadPhotosAction: submit photo files
uploadPhotosAction->>createPhotos: pass file bytes and metadata
createPhotos-->>uploadPhotosAction: return upload results
uploadPhotosAction-->>FirearmPhotos: refresh gallery
FirearmPhotos->>PhotoServingRoute: request photo variant
PhotoServingRoute-->>FirearmPhotos: return authenticated image bytes
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
✨ Simplify code
Warning Review ran into problems🔥 ProblemsThese MCP integrations need to be re-authenticated in the Integrations settings: Notion Comment |
coderabbitai
Bot
added
backend
dependencies
There was a problem hiding this comment.
Pull request overview
Implements firearm photo management on top of a reusable local-filesystem storage abstraction, including upload + processing (derivatives + metadata stripping), authenticated serving endpoints, firearm-scoped authorization, and UI surfaces for gallery management and list thumbnails.
Changes:
- Added a backend-agnostic
StorageServicewith a local-filesystem adapter,UPLOAD_DIRconfig, and docker-compose volume wiring. - Introduced
firearm_photopersistence (schema + migration) plus a domain service for upload/list/reorder/caption/delete/primary, with asharp-based processing pipeline. - Wired authenticated photo serving and UI integration (detail gallery + list thumbnails) with unit/integration/e2e coverage and documentation.
Reviewed changes
Copilot reviewed 42 out of 45 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test-support/factories.ts | Adds makeFirearmPhoto factory for integration tests. |
| src/storage/service.ts | Defines storage interface contract and key type. |
| src/storage/photo-blobs.ts | Best-effort deletion for original + derivative photo blobs. |
| src/storage/orphan-sweep.ts | Adds on-demand orphaned-blob sweep utility for photo storage. |
| src/storage/local-fs-adapter.ts | Implements local filesystem storage with traversal protection. |
| src/storage/keys.ts | Adds server-generated key + derivative key conventions. |
| src/storage/index.ts | Exposes a lazy-initialized shared storage singleton and helpers. |
| src/storage/env.ts | Validates UPLOAD_DIR configuration at runtime. |
| src/storage/tests/local-fs-adapter.test.ts | Tests adapter behavior (round-trip, delete, traversal, env validation). |
| src/domain/firearms/service.ts | Wires firearm delete to best-effort photo blob cleanup via pre-delete hook. |
| src/domain/firearms/tests/service.test.ts | Adds regression tests for firearm-delete blob cleanup + orphan sweep. |
| src/domain/firearm-photos/validate.ts | Adds pure validation for MIME/size and batch-size caps. |
| src/domain/firearm-photos/urls.ts | Adds client-safe URL builder for authenticated photo variants. |
| src/domain/firearm-photos/service.ts | Implements photo CRUD domain logic, authz-through-parent, and batched thumbnail lookup. |
| src/domain/firearm-photos/pipeline.ts | Adds sharp pipeline for re-encode, metadata stripping, and derivatives. |
| src/domain/firearm-photos/constants.ts | Centralizes allow-list and size/pixel/count/derivative constants. |
| src/domain/firearm-photos/tests/validate.test.ts | Unit tests for upload validation and batch-size enforcement. |
| src/domain/firearm-photos/tests/serving.test.ts | Integration tests for getServablePhoto authz and variant resolution. |
| src/domain/firearm-photos/tests/service.test.ts | Integration tests for create/setPrimary/reorder/delete/quota/batched lookup. |
| src/domain/firearm-photos/tests/pipeline.test.ts | Unit tests for derivative sizing, EXIF stripping, pixel cap, and format confusion rejection. |
| src/db/migrations/meta/0014_snapshot.json | Drizzle snapshot for migration 0014. |
| src/db/migrations/meta/_journal.json | Registers migration 0014 in the journal. |
| src/db/migrations/0014_perfect_silver_surfer.sql | Creates firearm_photo table + FK + index + check. |
| src/db/inventory-schema.ts | Adds firearmPhoto table schema to inventory DB model. |
| src/auth/authorize.ts | Extends authorizeAndDeleteParent with optional in-transaction pre-delete hook. |
| package.json | Adds sharp dependency and allows install scripts to run for it. |
| bun.lock | Locks new sharp dependency graph. |
| docker-compose.yml | Adds uploads volume and UPLOAD_DIR env for the app container. |
| .env.example | Documents UPLOAD_DIR configuration for local/Docker setups. |
| app/api/photos/[id]/[variant]/route.ts | Adds authenticated photo serving route with pinned Content-Type and nosniff. |
| app/(app)/firearms/photo-actions.ts | Adds server actions for upload/mutations with rate limiting + revalidation. |
| app/(app)/firearms/page.tsx | Loads primary thumbnails in a batched lookup for list rendering. |
| app/(app)/firearms/firearms-view.tsx | Adds a photo thumbnail column to the firearms list/table UI. |
| app/(app)/firearms/firearm-detail-view.tsx | Embeds the detail-view photo gallery section. |
| app/(app)/firearms/[id]/page.tsx | Loads ordered photos for the firearm detail page. |
| app/(app)/firearms/[id]/firearm-photos.tsx | Implements client gallery UI: upload, per-file failures, primary, reorder, caption, delete, empty state. |
| components/ui/photo-thumbnail.tsx | Adds reusable list/search thumbnail UI with fixed footprint placeholder. |
| e2e/start-test-server.ts | Sets an ephemeral UPLOAD_DIR for e2e runs. |
| e2e/fixtures/user-pool.ts | Adds e2e users for photo and thumbnail specs. |
| e2e/fixtures/not-an-image.txt | Adds invalid upload fixture for mixed-validity upload coverage. |
| e2e/firearm-photos.spec.ts | E2E coverage for gallery workflows including mixed-validity uploads and keyboard reorder. |
| e2e/firearm-list-thumbnails.spec.ts | E2E coverage for primary thumbnails and placeholder behavior in list view. |
| docs/plans/2026-07-09-001-feat-firearm-photo-management-plan.md | Adds implementation plan and contracts for the feature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/domain/firearm-photos/__tests__/pipeline.test.ts (1)
1-142: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNo happy-path coverage for PNG/WebP/AVIF — only JPEG is exercised.
Every fixture (
makeJpegFixture,makeJpegFixtureWithGpsExif) and the format-confusion test useimage/jpeg/JPEG bytes. SinceALLOWED_MIME_TYPESincludespng,webp, andavif, and given the AVIF format-detection mismatch flagged inconstants.ts(sharp reports AVIF asformat: "heif", not"avif"), a test assertingprocessImagesucceeds for a real AVIF fixture would have caught that regression before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/firearm-photos/__tests__/pipeline.test.ts` around lines 1 - 142, Add happy-path coverage in the processImage tests for real PNG, WebP, and AVIF fixtures, invoking processImage with each corresponding MIME type and asserting successful original, thumbnail, preview, and dimension outputs. Include an AVIF fixture and verify format detection accepts sharp’s reported “heif” format, using the existing fixture helpers or adding format-specific helpers as needed.
🧹 Nitpick comments (3)
src/storage/keys.ts (1)
8-10: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winValidate
extbefore embedding it in a storage key.Values such as
../../etc/passwdorfoo/barproduce path-bearing keys. The current local adapter rejects some escaped paths, but this safety depends on every backend and caller. Use an allowlisted extension/MIME type or reject separators,.., and empty values; verify callers never derive it from an upload filename.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/storage/keys.ts` around lines 8 - 10, Validate the ext argument in generateKey before constructing the StorageKey: reject empty values, path separators, and traversal segments such as "..", or enforce an allowlisted extension/MIME mapping. Ensure callers pass a validated type rather than deriving it directly from upload filenames, and add coverage for invalid path-bearing inputs.src/domain/firearm-photos/pipeline.ts (1)
39-113: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winEach upload is decoded 4 separate times (
metadata()+ 3×reencode).
reencodebuilds a freshsharp(bytes, ...)per output instead of decoding once and using.clone(). Per sharp's own docs,.clone()is designed exactly for this: "Cloned instances inherit the input of their parent instance. This allows multiple output Streams and therefore multiple processing pipelines to share a single input" — libvips decodes once and shares the decoded image across clones, whereas separatesharp()calls each pay the full decode cost independently.The doc comment on
reencode(lines 91-96) frames the fresh-instance choice as necessary for "independent and side-effect free" pipelines, but.clone()gives that same independence (each clone can be.resize()'d/.toFormat()'d differently) without the redundant decode — it doesn't introduce shared mutable state across the derivative outputs. Since this runs synchronously on the request thread for up toMAX_FILES_PER_REQUEST(10) files per upload, cutting 4 decodes down to 1 is a meaningful reduction in CPU/latency on this hot path.♻️ Proposed fix — decode once, clone for each derivative
export async function processImage( bytes: Uint8Array | Buffer, mimeType: string, ): Promise<ProcessedImage> { - const metadata = await sharp(bytes, { - limitInputPixels: MAX_INPUT_PIXELS, - }).metadata(); + const pipeline = sharp(bytes, { limitInputPixels: MAX_INPUT_PIXELS }); + const metadata = await pipeline.metadata(); ... const [original, thumb, preview] = await Promise.all([ - reencode(bytes, mimeType).toBuffer(), - reencode(bytes, mimeType) + reencode(pipeline, mimeType).toBuffer(), + reencode(pipeline, mimeType) .resize({ width: THUMB_MAX_EDGE, height: THUMB_MAX_EDGE, fit: "inside", withoutEnlargement: true }) .toBuffer(), - reencode(bytes, mimeType) + reencode(pipeline, mimeType) .resize({ width: PREVIEW_MAX_EDGE, height: PREVIEW_MAX_EDGE, fit: "inside", withoutEnlargement: true }) .toBuffer(), ]); ... } -function reencode(bytes: Uint8Array | Buffer, mimeType: string): Sharp { - const pipeline = sharp(bytes, { limitInputPixels: MAX_INPUT_PIXELS }); +function reencode(pipeline: Sharp, mimeType: string): Sharp { + const cloned = pipeline.clone(); switch (mimeType) { case "image/jpeg": - return pipeline.jpeg(); + return cloned.jpeg(); case "image/png": - return pipeline.png(); + return cloned.png(); case "image/webp": - return pipeline.webp(); + return cloned.webp(); case "image/avif": - return pipeline.avif(); + return cloned.avif(); default: throw new Error(`firearm-photos/pipeline: unsupported mime type "${mimeType}"`); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/firearm-photos/pipeline.ts` around lines 39 - 113, Update processImage to create one base sharp pipeline, trigger a single decode, and derive original, thumb, and preview outputs from independent clones of that pipeline. Replace the current three reencode calls and remove or revise reencode so format-specific encoding remains applied per clone while resize settings stay independent; preserve the existing pixel limit, output formats, and returned dimensions.app/(app)/firearms/[id]/firearm-photos.tsx (1)
365-372: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffStatic analysis flags
<img>overnext/image.Biome's
noImgElementwarning applies here, but swapping tonext/imagefor these authenticated, dynamically-sized photo variants would need a custom loader/remote-pattern config for the/api/photos/[id]/[variant]route — the manualwidth/heightreservation already prevents layout shift (R17). Given the added config complexity versus the marginal gain, this is optional rather than a must-fix.Also applies to: 409-416
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/`(app)/firearms/[id]/firearm-photos.tsx around lines 365 - 372, Static analysis flags the native img elements in the primary and secondary photo rendering blocks, but this is an optional warning rather than a required fix. Leave the existing img usage unchanged unless choosing to migrate; if migrated, update both occurrences to next/image and provide a loader or remote-pattern configuration supporting authenticated dynamic photoVariantUrl routes while preserving the explicit dimensions and layout behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/`(app)/firearms/[id]/page.tsx:
- Around line 38-50: Wrap the Promise.all block containing listPhotos in the
same NotFoundError catch used for getFirearm, and call notFound() when that
error is caught. Ensure errors from listPhotos during the permission race
produce the page’s expected 404 while preserving existing handling for other
errors.
In `@app/`(app)/firearms/photo-actions.ts:
- Around line 47-78: Raise the Server Actions request body limit in
next.config.ts by configuring experimental.serverActions.bodySizeLimit to
accommodate the maximum allowed upload batch, including up to 15 MB per file and
any permitted number of files. Ensure the setting matches the limits enforced by
uploadPhotosAction and related upload validation.
In `@app/api/photos/`[id]/[variant]/route.ts:
- Around line 37-72: Add a small per-user rate limiter to the GET handler,
placing it after getCurrentUser() confirms authentication and before UUID
validation or getServablePhoto(). Reuse the existing limiter utility and
conventions from the mutation routes, keying the limit by user.id and returning
the established throttling response (including any Retry-After metadata) when
exceeded.
In `@docs/plans/2026-07-09-001-feat-firearm-photo-management-plan.md`:
- Around line 239-241: Update the firearm-photo management plan’s test scenarios
and verification sections to require Testcontainers for integration tests
instead of a caller-provided DATABASE_URL or skipping tests when it is absent.
Ensure the planned tests provision and clean up an isolated database container
for each test suite or run, including the referenced verification guidance.
- Around line 38-49: Update requirement R1 and the storage interface contract to
expose the operations required by orphan sweeping, including existence checks
and listing stored objects/keys (with any needed metadata). Ensure U5 and
related requirements explicitly use these abstraction methods rather than
bypassing the storage service, and keep the contract backend-independent for
filesystem and future S3 adapters.
- Around line 48-49: Update the deletion flow described by R8 and R19 so firearm
photo blob deletion occurs only after the database transaction commits, never
before row deletion. Use a post-commit cleanup hook, or a durable
pending-delete/outbox mechanism for retryable asynchronous cleanup, and ensure
failures remain recoverable without deleting blobs when the transaction rolls
back. Apply the same change to the corresponding sections noted in the review.
In `@e2e/firearm-photos.spec.ts`:
- Around line 20-164: Extend the firearm photo E2E coverage with a second-user
authorization scenario using the existing sharing flow: verify a view-only user
cannot access or delete photos, while an edit-grantee can upload, reorder, and
delete them. Reuse the photo gallery and unique controls from the existing test,
and assert both allowed and denied behaviors for R12/R14 and AE2/AE3.
In `@e2e/start-test-server.ts`:
- Around line 137-142: Retain the path returned by mkdtempSync in e2e server
startup and add teardown logic using rmSync(path, { recursive: true, force: true
}). Invoke this cleanup before every process.exit, including container-start
failure and main().catch paths, using the relevant startup and error-handling
functions.
In `@src/db/inventory-schema.ts`:
- Around line 344-368: Add a partial unique index enforcing at most one primary
photo per firearm, using the firearm photo schema/migration definitions around
firearmPhoto and the isPrimary field. Create the unique index on firearm_id with
a WHERE is_primary predicate, and include the corresponding migration so
existing and future databases enforce this constraint.
In `@src/domain/firearm-photos/constants.ts`:
- Around line 28-51: AVIF files are rejected because sharp reports their format
as “heif” rather than “avif”. Update isAllowedRasterFormat and its callers,
including processImage, to accept AVIF only when metadata identifies HEIF with
AV1 compression or the appropriate media type, while preserving rejection of
unsupported HEIF variants; alternatively remove image/avif from
ALLOWED_MIME_TYPES until AVIF is supported.
In `@src/domain/firearm-photos/service.ts`:
- Around line 118-157: Move image processing and blob writes out of the database
transaction enclosing the batch loop in the firearm photo service. Refactor the
workflow around the batch transaction so each file’s processing and storage
saves occur before the corresponding database insert, with cleanup for
already-written blobs when insertion or later transaction work fails;
alternatively implement the planned per-file isolation to prevent orphaned
storage and reduce transaction duration. Use the existing processImage,
storage.save, and firearmPhoto insert flow as the refactoring points.
- Around line 95-116: Prevent concurrent createPhotos transactions from
bypassing the photo quota and primary-photo invariant. In createPhotos,
serialize operations per firearm by locking the firearm row with SELECT ... FOR
UPDATE or using a serializable transaction before reading existing photos;
alternatively add and handle a database partial unique constraint for primary
photos together with quota-safe locking. Ensure the protected section covers
existing, the MAX_PHOTOS_PER_FIREARM check, nextSortOrder, hasPrimary, and all
inserts.
- Around line 270-294: Update reorderPhotos to validate that orderedPhotoIds
exactly matches the firearm’s current photo-id set before performing updates:
reject duplicates, missing existing IDs, and unknown IDs with the appropriate
validation failure codes. Load the current IDs within the transaction, compare
sets and lengths, surface all applicable validation errors, and only then
execute the sortOrder updates.
In `@src/domain/firearms/__tests__/service.test.ts`:
- Around line 1-11: Make storage tests use a single shared upload root instead
of assigning separate module-scoped temporary directories in each test file.
Update the setup in the firearm photo service/serving tests and the firearms
service test, and ensure the lazy singleton accessed through activeStorageRoot()
and orphan-sweep assertions consistently uses that shared root throughout the
test run.
In `@src/domain/firearms/service.ts`:
- Around line 101-138: Refactor cleanupFirearmPhotoBlobs to only collect and
return the firearm photo storage keys during the transaction, without calling
deletePhotoBlobs. Update deleteFirearm to await authorizeAndDeleteParent, then
delete the collected blobs after it resolves, preserving best-effort failure
handling and ensuring filesystem I/O occurs post-commit.
In `@src/storage/local-fs-adapter.ts`:
- Around line 45-48: Update LocalFsAdapter.save to create upload directories
with mode 0o700 and files with mode 0o600, rather than relying on the process
umask. Also ensure the existing upload root is tightened to 0o700 when
applicable, including handling pre-existing paths safely.
In `@src/storage/orphan-sweep.ts`:
- Around line 38-40: Update the directory read in the orphan-sweep function to
treat only an ENOENT failure as an empty upload directory; rethrow or otherwise
surface all other errors such as permission and I/O failures. Preserve the
existing successful empty-result behavior for a missing directory while ensuring
callers receive non-ENOENT failures.
In `@src/storage/photo-blobs.ts`:
- Around line 10-17: Move the blob deletion performed by
cleanupFirearmPhotoBlobs out of the firearm-delete transaction: collect and
return the affected storage keys while the transaction runs, then invoke the
deletion helper only after the transaction commits; update related documentation
and error handling to reflect the new timing.
---
Outside diff comments:
In `@src/domain/firearm-photos/__tests__/pipeline.test.ts`:
- Around line 1-142: Add happy-path coverage in the processImage tests for real
PNG, WebP, and AVIF fixtures, invoking processImage with each corresponding MIME
type and asserting successful original, thumbnail, preview, and dimension
outputs. Include an AVIF fixture and verify format detection accepts sharp’s
reported “heif” format, using the existing fixture helpers or adding
format-specific helpers as needed.
---
Nitpick comments:
In `@app/`(app)/firearms/[id]/firearm-photos.tsx:
- Around line 365-372: Static analysis flags the native img elements in the
primary and secondary photo rendering blocks, but this is an optional warning
rather than a required fix. Leave the existing img usage unchanged unless
choosing to migrate; if migrated, update both occurrences to next/image and
provide a loader or remote-pattern configuration supporting authenticated
dynamic photoVariantUrl routes while preserving the explicit dimensions and
layout behavior.
In `@src/domain/firearm-photos/pipeline.ts`:
- Around line 39-113: Update processImage to create one base sharp pipeline,
trigger a single decode, and derive original, thumb, and preview outputs from
independent clones of that pipeline. Replace the current three reencode calls
and remove or revise reencode so format-specific encoding remains applied per
clone while resize settings stay independent; preserve the existing pixel limit,
output formats, and returned dimensions.
In `@src/storage/keys.ts`:
- Around line 8-10: Validate the ext argument in generateKey before constructing
the StorageKey: reject empty values, path separators, and traversal segments
such as "..", or enforce an allowlisted extension/MIME mapping. Ensure callers
pass a validated type rather than deriving it directly from upload filenames,
and add coverage for invalid path-bearing inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 27cef71d-3164-44d3-9e91-b29e8b60ec6c
⛔ Files ignored due to path filters (4)
.env.exampleis excluded by!.env*bun.lockis excluded by!**/*.lock,!bun.locke2e/fixtures/sample-photo-1.jpgis excluded by!**/*.jpge2e/fixtures/sample-photo-2.jpgis excluded by!**/*.jpg
📒 Files selected for processing (41)
app/(app)/firearms/[id]/firearm-photos.tsxapp/(app)/firearms/[id]/page.tsxapp/(app)/firearms/firearm-detail-view.tsxapp/(app)/firearms/firearms-view.tsxapp/(app)/firearms/page.tsxapp/(app)/firearms/photo-actions.tsapp/api/photos/[id]/[variant]/route.tscomponents/ui/photo-thumbnail.tsxdocker-compose.ymldocs/plans/2026-07-09-001-feat-firearm-photo-management-plan.mde2e/firearm-list-thumbnails.spec.tse2e/firearm-photos.spec.tse2e/fixtures/not-an-image.txte2e/fixtures/user-pool.tse2e/start-test-server.tspackage.jsonsrc/auth/authorize.tssrc/db/inventory-schema.tssrc/db/migrations/0014_perfect_silver_surfer.sqlsrc/db/migrations/meta/0014_snapshot.jsonsrc/db/migrations/meta/_journal.jsonsrc/domain/firearm-photos/__tests__/pipeline.test.tssrc/domain/firearm-photos/__tests__/service.test.tssrc/domain/firearm-photos/__tests__/serving.test.tssrc/domain/firearm-photos/__tests__/validate.test.tssrc/domain/firearm-photos/constants.tssrc/domain/firearm-photos/pipeline.tssrc/domain/firearm-photos/service.tssrc/domain/firearm-photos/urls.tssrc/domain/firearm-photos/validate.tssrc/domain/firearms/__tests__/service.test.tssrc/domain/firearms/service.tssrc/storage/__tests__/local-fs-adapter.test.tssrc/storage/env.tssrc/storage/index.tssrc/storage/keys.tssrc/storage/local-fs-adapter.tssrc/storage/orphan-sweep.tssrc/storage/photo-blobs.tssrc/storage/service.tssrc/test-support/factories.ts
…ing (#9) From the comprehensive PR review of #62: - partial unique index (migration 0015) enforcing one primary photo per firearm - negative authz tests for delete/setPrimary/reorder/setCaption mutations - e2e assertions for the serving route's Content-Type + nosniff + unauth status - log (not swallow) unexpected processImage and orphan-sweep readdir failures - thread AllowedMimeType through the pipeline; positive-invariant comment Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
coderabbitai
Bot
removed
documentation
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/domain/firearm-photos/service.ts`:
- Around line 144-148: Update the error logging in the processImage failure
catch block to use a constant format string instead of interpolating firearmId
or input.mimeType. Pass those request-derived values as separate structured
metadata arguments to console.error, preserving the existing context and error
object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 03003576-b737-4294-a0fe-7bf993973ec3
📒 Files selected for processing (10)
e2e/firearm-list-thumbnails.spec.tssrc/db/inventory-schema.tssrc/db/migrations/0015_sleepy_northstar.sqlsrc/db/migrations/meta/0015_snapshot.jsonsrc/db/migrations/meta/_journal.jsonsrc/domain/firearm-photos/__tests__/service.test.tssrc/domain/firearm-photos/pipeline.tssrc/domain/firearm-photos/service.tssrc/storage/orphan-sweep.tssrc/storage/photo-blobs.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/db/inventory-schema.ts
- src/storage/photo-blobs.ts
- e2e/firearm-list-thumbnails.spec.ts
- src/db/migrations/meta/_journal.json
- src/domain/firearm-photos/pipeline.ts
- src/storage/orphan-sweep.ts
- src/domain/firearm-photos/tests/service.test.ts
CodeQL js/tainted-format-string: the processImage-failure log interpolated the user-supplied mimeType into console.error's format-string position. Move all dynamic values to a structured argument with a static message. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Second-pass review surfaced issues green CI missed because tests never
exercise a real deployment or a file over ~278 bytes:
- P0: raise Server Action body cap (next.config) — Next defaults to 1MB,
which rejected every real photo (2-8MB) before the upload action ran.
Sized 160mb for a full advertised batch (10 x 15MB) + multipart overhead.
- P0: create /data/uploads owned by uid `bun` in the image so the named
uploads volume mounts writable — first upload otherwise failed EACCES on
the documented docker compose path (USER bun, root-owned fresh volume).
- P1: delete firearm photo blobs AFTER the delete transaction commits,
mirroring deletePhoto — pre-commit deletion could strip bytes a
rolled-back (still-live) row points at, unreclaimable by orphanSweep.
- P1: lock the parent firearm row (.for("update")) in createPhotos so the
per-firearm quota read + inserts are atomic against a concurrent upload,
matching updateMagazine — closes the R20 disk-exhaustion race.
Also corrects the photo-blobs doc comment that claimed the firearm-delete
path used safe pre-commit timing.
Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
coderabbitai
Bot
added
the
documentation
There was a problem hiding this comment.
🧹 Nitpick comments (1)
next.config.ts (1)
6-16: 🧹 Nitpick | 🔵 Trivial
bodySizeLimitis global to every Server Action, not just uploads.Raising this to
160mbapplies to all Server Actions in the app, so any authenticated action now accepts bodies up to 160MB.uploadPhotosActionis protected byuploadLimiter, but other actions gain the larger body ceiling without that per-action bound. The config key/placement is correct for Next 16 (experimental.serverActions.bodySizeLimit, string sizes accepted), so this is an operational hardening note rather than a defect: consider keeping heavy uploads on a dedicated route/action and relying on the per-action rate limiter as the effective backstop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@next.config.ts` around lines 6 - 16, Keep the Next.js serverActions.bodySizeLimit configuration valid, but avoid relying on its global 160MB ceiling for every action: move heavy photo uploads to a dedicated route or action where practical, and ensure uploadPhotosAction continues enforcing uploadLimiter and the existing file/count limits. Review other Server Actions for equivalent request-size validation or explicit rejection so they do not implicitly accept oversized bodies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@next.config.ts`:
- Around line 6-16: Keep the Next.js serverActions.bodySizeLimit configuration
valid, but avoid relying on its global 160MB ceiling for every action: move
heavy photo uploads to a dedicated route or action where practical, and ensure
uploadPhotosAction continues enforcing uploadLimiter and the existing file/count
limits. Review other Server Actions for equivalent request-size validation or
explicit rejection so they do not implicitly accept oversized bodies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f0cefc63-40b4-45fa-9290-0d51053667d7
📒 Files selected for processing (5)
Dockerfilenext.config.tssrc/domain/firearm-photos/service.tssrc/domain/firearms/service.tssrc/storage/photo-blobs.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/storage/photo-blobs.ts
- src/domain/firearm-photos/service.ts
Medium-severity hardening from the second-pass review: - Add DB CHECK backstops on firearm_photo (migration 0016): mime_type in the controlled allow-list, and size_bytes/width/height > 0 — a direct insert bypassing the app layer can no longer write a bad MIME (echoed as Content-Type) or a degenerate dimension. Mirrors grant_permission_valid. - Export CreatePhotoErrorCode / PhotoServiceErrorCode as the single source of truth for the photo failure vocabulary; the throw sites now type-check against it (satisfies) and the upload UI imports it instead of re-deriving the set by hand (which was already missing a code). - Make PHOTO_VARIANTS (urls.ts) the one servable-variant list; the serving route and the server-side PhotoVariant derive from it instead of each retyping the literal set. - Drop the redundant, unvalidated CreatePhotoInput.sizeBytes — createPhotos derives it from the actual buffer length (the two can no longer drift). - Replace reencode's runtime-default switch with a compiler-exhaustive Record<AllowedMimeType, ...> so a new format fails to typecheck until wired, rather than throwing at runtime. Tests: add the previously-missing negative/edge cases — createPhotos' processingFailed catch path (allowed-MIME undecodable bytes; survivors stay gapless, one primary), listPhotos existence-hiding for a stranger, and reorderPhotos' cross-firearm-id no-op guard. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
coderabbitai
Bot
removed
shared
documentation
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/db/inventory-schema.ts (1)
375-378: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a drift guard for the triplicated MIME allow-list.
The comment correctly flags this SQL literal as a third copy of
ALLOWED_MIME_TYPES. Since the serving route echoes these values asContent-Type, silent drift here has a security edge. A one-line assertion test that the DDL literal matches the TS constant makes the drift compile/CI-visible instead of relying on the comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/inventory-schema.ts` around lines 375 - 378, Add a one-line assertion test that extracts the MIME values from the `firearm_photo_mime_type_valid` SQL check in `inventory-schema.ts` and verifies they exactly match the `ALLOWED_MIME_TYPES` TypeScript constant, preserving order or comparing normalized sets consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/db/migrations/0016_bizarre_lyja.sql`:
- Around line 1-4: The migration immediately validates four CHECK constraints
against all existing firearm_photo rows. Update the constraints named
firearm_photo_mime_type_valid, firearm_photo_size_bytes_min,
firearm_photo_width_min, and firearm_photo_height_min to be added as NOT VALID,
add a data-fix step for existing invalid rows, and create a follow-up migration
that runs VALIDATE CONSTRAINT for each constraint.
---
Nitpick comments:
In `@src/db/inventory-schema.ts`:
- Around line 375-378: Add a one-line assertion test that extracts the MIME
values from the `firearm_photo_mime_type_valid` SQL check in
`inventory-schema.ts` and verifies they exactly match the `ALLOWED_MIME_TYPES`
TypeScript constant, preserving order or comparing normalized sets consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5c7eb2c5-5d4e-40af-a78a-f84438fa81a5
📒 Files selected for processing (12)
app/(app)/firearms/[id]/firearm-photos.tsxapp/(app)/firearms/photo-actions.tsapp/api/photos/[id]/[variant]/route.tssrc/db/inventory-schema.tssrc/db/migrations/0016_bizarre_lyja.sqlsrc/db/migrations/meta/0016_snapshot.jsonsrc/db/migrations/meta/_journal.jsonsrc/domain/firearm-photos/__tests__/service.test.tssrc/domain/firearm-photos/__tests__/serving.test.tssrc/domain/firearm-photos/pipeline.tssrc/domain/firearm-photos/service.tssrc/domain/firearm-photos/urls.ts
💤 Files with no reviewable changes (1)
- app/(app)/firearms/photo-actions.ts
✅ Files skipped from review due to trivial changes (1)
- src/db/migrations/meta/0016_snapshot.json
🚧 Files skipped from review as they are similar to previous changes (8)
- src/domain/firearm-photos/urls.ts
- src/db/migrations/meta/_journal.json
- app/api/photos/[id]/[variant]/route.ts
- src/domain/firearm-photos/tests/serving.test.ts
- src/domain/firearm-photos/pipeline.ts
- src/domain/firearm-photos/tests/service.test.ts
- app/(app)/firearms/[id]/firearm-photos.tsx
- src/domain/firearm-photos/service.ts
- Accept AVIF uploads: sharp reports AVIF's container format as "heif", so gating processImage on metadata.format rejected every valid AVIF. Gate on the magic-byte mediaType (image/avif) instead — still rejects SVG/format confusion. Adds an AVIF end-to-end pipeline test. - Store upload dirs 0700 / blobs 0600 instead of inheriting the umask, so private photos aren't world-readable on a self-hosted UPLOAD_DIR. - orphanSweep is now best-effort per key: one delete failure no longer aborts the whole reclaim run. - Photo-serving route returns a zero-copy Uint8Array view instead of copying every image byte per request. - Firearm detail page wraps listPhotos in the same NotFoundError -> notFound() guard as getFirearm (permission-revoke race -> 404, not 500). - e2e launcher removes its ephemeral UPLOAD_DIR on all exit paths. - Test hygiene: factory storageKey matches production shape (flat, extension, no separator); photo-blob disk assertions read activeStorageRoot() so they don't depend on test-file load order. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Fully resolves the three deferred follow-ups rather than deferring them. createPhotos — per-file isolation + processing out of the transaction: - Decode/re-encode and blob writes now run per file OUTSIDE any transaction, so a validation, processing, OR storage failure on one file becomes that file's result without aborting the batch or holding a pool connection during CPU/IO work. A partial blob write is cleaned up in place. - Only successful rows insert inside a short transaction that re-authorizes, locks the firearm row, and re-checks the quota (race-safe). An optimistic unlocked quota check still fails an over-limit batch before any processing. - If that transaction rolls back, the batch's already-written blobs are deleted directly — self-cleaning, not reliant on the orphan sweep. orphanSweep — in-flight-upload guard: - Adds an mtime grace period (ORPHAN_MIN_AGE_MS, 1h; injectable via minAgeMs) so a blob written just before its row commits is never reclaimed. Age is clamped >= 0 so sub-ms mtime skew can't skip a blob at minAgeMs: 0. - Reports skippedRecentCount. Tests: orphan-sweep reclaim test pins minAgeMs: 0; adds a grace-period test proving a recently-written unreferenced blob is spared. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
unclesp1d3r
deleted the
9-add-firearm-photo-management-with-reusable-uploadstorage-support
branch
July 11, 2026 17:19
10 tasks
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
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.
Summary
Adds firearm photo management (#9) on a reusable upload/media foundation: multi-photo upload, a thumbnail gallery, reorder, captions, delete, and a designated primary image shown on the firearm detail view and as a list/search thumbnail. Photos are a firearm child-record family (owner/grants inherited through the parent firearm), processed synchronously on upload, and served only through authenticated, authorization-checked endpoints.
bun.lock) or tests (~1,400) — safe to skim.sharp, but isolated behind a child-record seam and reviewed across three passes (two multi-agent reviews + bot feedback), all applied.ci,e2e,CodeQL,Analyze,CodeRabbit,DCOall greenPlanned via
/ce-brainstorm→/ce-plan(plan:docs/plans/2026-07-09-001-feat-firearm-photo-management-plan.md) and implemented across 8 units.Suggested review order
The feature reads cleanly bottom-up — each layer only depends on the ones above it:
src/storage/service.ts(interface) →local-fs-adapter.ts(traversal guard,0700/0600modes) →keys.ts/index.ts(lazy singleton).src/db/inventory-schema.ts(firearmPhoto) + migrations0014–0016(table, single-primary partial unique index, CHECK backstops). (Skip themeta/*.jsonsnapshots — generated.)src/domain/firearm-photos/:constants.ts→validate.ts→pipeline.ts(sharp) →service.ts(the core; authz-through-parent, primary invariant, quota lock).src/auth/authorize.ts(onBeforeDeletehook) +src/domain/firearms/service.ts(post-commit blob cleanup).app/(app)/firearms/photo-actions.ts(server actions) →app/api/photos/[id]/[variant]/route.ts(serving) →firearm-photos.tsxgallery + list thumbnails.What's included
StorageServiceinterface + local-filesystem adapter (traversal-safe server-generated keys, derivative-key convention, owner-only modes),UPLOAD_DIRconfig, uploads volume indocker-compose.ymlfirearm_photochild table + migrations (0014table,0015single-primary partial unique index,0016mime/positivity CHECKs)sharp: MIME/size/batch/pixel validation, EXIF/XMP/IPTC location-metadata stripping (via re-encode), thumbnail/preview derivatives, magic-bytemediaTypegating (accepts AVIF, rejects SVG-SSRF)authorizeAndDeleteParent(magazine/ammo delete unaffected) + orphan sweep/api/photos/[id]/[variant]route withContent-Typepinned from the stored type +nosniff+ immutable cacheChange map
src/storage/*,src/domain/firearm-photos/*, serving route, server actions, gallery + thumbnail UI,authorize.tshook,firearms/service.ts, schema.next.config.ts(Server Action body limit),Dockerfile(uploads dir ownership),docker-compose.yml(uploads volume),.env.example.Key decisions
UPLOAD_DIRvolume, no new test container). An S3-compatible adapter is a later drop-in behind the same interface.owner_id/grants; authz resolves through the parent firearm).sharpprocessing — re-encoding strips EXIF/XMP/IPTC by default;limitInputPixelscaps decompression bombs; per-request file cap bounds request time.Content-Type+nosniff, and hides existence (404, never 403).Security posture
0600, dirs0700.mediaTypegating before rasterization — closes an SVG-via-image/pngSSRF vector against the internal network while correctly accepting AVIF.Review checklist
0014–0016are additive on a new table (no backfill/legacy-row concern).Test plan
just ci-checkpasses end to end: Biome lint + format,tsc, pre-commit hooks, the fullbun testunit/integration suite (photo storage/pipeline/service/serving + firearm-delete regression + AVIF regression + negative authz), and the Playwright e2e suite (e2e/firearm-photos.spec.ts,e2e/firearm-list-thumbnails.spec.ts) covering upload, mixed-validity batches, set-primary, keyboard reorder, delete + auto-promotion, empty state, list thumbnails, and serving-route headers.Second-pass review — fixes applied
A follow-up multi-agent review + bot feedback caught issues the first pass and green CI missed (the tests never exercise a real deployment or a file over ~278 bytes). All fixed on this branch:
sharpreports AVIF's container format as"heif"; the guard now uses the magic-bytemediaType, with an AVIF regression test.next.confignow setsserverActions.bodySizeLimit./data/uploadsowned bybunso the named volume mounts writable (wasEACCES).deletePhoto)..for("update").0600/0700modes, best-effort orphan sweep, zero-copy serving response,listPhotos404 guard, e2e temp-dir cleanup.Residual follow-ups — resolved
The three previously-deferred items are now fixed on this branch (commit
8505d1b):createPhotosper-file isolation + processing out of the transaction — sharp decode/encode and blob writes now run per file outside any transaction, so a validation, processing, or storage failure on one file becomes that file's result without aborting the batch or holding a pool connection during CPU/IO. Only successful rows insert in a short transaction that re-authorizes, locks the firearm row, and re-checks the quota; on rollback the batch's blobs are deleted directly (self-cleaning).orphanSweepin-flight-upload guard — an mtime grace period (ORPHAN_MIN_AGE_MS, 1 h; injectable) so a blob written just before its row commits is never reclaimed; age clamped ≥ 0 against sub-ms mtime skew. ReportsskippedRecentCount, with a test proving a recent unreferenced blob is spared.No known residuals remain.