Implement photo processing library and uploads streaming route #4

New Issue

marcel · 2026-05-05T10:57:03+02:00

marcel commented

2026-05-05 10:57:03 +02:00

Task 4 — Plan reference: `docs/superpowers/plans/2026-05-05-erbstuecke-wannsee.md`

User story: As an admin, I want uploaded photos to be automatically resized and converted to WebP so that the gallery loads fast on mobile; and as a developer I need a route to stream those photos to the browser.

Acceptance criteria

processAndSavePhoto(buffer, artikelId) resizes to max 1200×1200px (preserving aspect ratio), converts to WebP quality 80, saves to $UPLOAD_DIR/{artikelId}/{uuid}.webp, returns relative filename
deletePhoto(filename) removes the file; does not throw if file is already gone
deleteArtikelPhotos(artikelId) removes the entire {artikelId}/ directory; does not throw if missing
resolveUploadPath(filename) returns the absolute path for streaming
GET /uploads/[...path] streams the WebP with Content-Type: image/webp and Cache-Control: public, max-age=31536000, immutable
Path traversal attack is blocked (resolved path must start with UPLOAD_DIR)
5 Vitest tests pass (saves WebP, correct filename pattern, delete removes file, delete tolerates missing file, deleteArtikelPhotos removes dir)

Files to create

src/lib/photos.ts
src/lib/photos.test.ts
src/routes/uploads/[...path]/+server.ts

NFR

NFR-PERF-002: Resulting WebP files must be ≤ ~800 KB (sharp quality 80 + 1200px limit achieves this for typical photos)
No authentication check on /uploads/ — URLs are UUID-based and not guessable (spec decision)

Depends on: #1 | Size: S | Spec: system-design §5

## Task 4 — Plan reference: `docs/superpowers/plans/2026-05-05-erbstuecke-wannsee.md` **User story:** As an admin, I want uploaded photos to be automatically resized and converted to WebP so that the gallery loads fast on mobile; and as a developer I need a route to stream those photos to the browser. ### Acceptance criteria - [ ] `processAndSavePhoto(buffer, artikelId)` resizes to max 1200×1200px (preserving aspect ratio), converts to WebP quality 80, saves to `$UPLOAD_DIR/{artikelId}/{uuid}.webp`, returns relative filename - [ ] `deletePhoto(filename)` removes the file; does not throw if file is already gone - [ ] `deleteArtikelPhotos(artikelId)` removes the entire `{artikelId}/` directory; does not throw if missing - [ ] `resolveUploadPath(filename)` returns the absolute path for streaming - [ ] `GET /uploads/[...path]` streams the WebP with `Content-Type: image/webp` and `Cache-Control: public, max-age=31536000, immutable` - [ ] Path traversal attack is blocked (resolved path must start with `UPLOAD_DIR`) - [ ] 5 Vitest tests pass (saves WebP, correct filename pattern, delete removes file, delete tolerates missing file, deleteArtikelPhotos removes dir) ### Files to create - `src/lib/photos.ts` - `src/lib/photos.test.ts` - `src/routes/uploads/[...path]/+server.ts` ### NFR - NFR-PERF-002: Resulting WebP files must be ≤ ~800 KB (sharp quality 80 + 1200px limit achieves this for typical photos) - No authentication check on `/uploads/` — URLs are UUID-based and not guessable (spec decision) **Depends on:** #1 | **Size:** S | **Spec:** system-design §5

marcel added this to the v1.0 — MVP milestone 2026-05-05 10:57:03 +02:00

marcel referenced this issue

2026-05-05 10:57:50 +02:00

Build Admin Inventar — article list and delete #9

marcel commented

2026-05-05 11:26:35 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

The plan's photos.ts uses sharp(...).toFile(dest) which is correct, but note the plan also shows .toBuffer() in the persona guide example — the toFile variant is the right choice here since we're writing to disk anyway (avoids a double-buffer).
uploadDir() is a private helper that reads process.env.UPLOAD_DIR ?? 'uploads' at call time — this is the right approach for test isolation (tests set process.env.UPLOAD_DIR = TEST_DIR in beforeEach). If the function were called at module-load time the test setup wouldn't work.
The test fixture uses tmpdir() + Date.now() for isolation — good. But the processAndSavePhoto tests call the real sharp, which requires a valid image buffer. The TINY_JPEG base64 constant is correct for this, but it creates a real filesystem dependency. This is intentional and acceptable �� photos.ts is inherently side-effectful.
The plan currently has 4 tests explicitly (saves WebP, delete removes file, delete tolerates missing, deleteArtikelPhotos removes dir, deleteArtikelPhotos tolerates missing) but the issue acceptance criteria says 5 tests pass — counting carefully: saves WebP + correct filename pattern is one test in the plan but the AC counts them separately. The "correct filename pattern" is covered implicitly by the filename regex assertion in the save test. Confirm whether the AC requires a dedicated second it() for the filename pattern.
deletePhoto silently catches all errors, not just ENOENT. If sharp writes a corrupted file that causes a permissions error, it will be silently swallowed. The AC says "does not throw if file is already gone" — a tighter catch would be if (e.code !== 'ENOENT') throw e.
The resolveUploadPath function is in photos.ts but has no test. The AC lists it — a one-liner test covering the return value would close this gap.

Recommendations

Tighten the deletePhoto catch to re-throw non-ENOENT errors:

} catch (e: unknown) {
  if ((e as NodeJS.ErrnoException).code !== 'ENOENT') throw e
}

Add a minimal test for resolveUploadPath:

it('resolveUploadPath returns absolute path for filename', () => {
  const abs = resolveUploadPath('42/abc.webp')
  expect(abs).toBe(join(TEST_DIR, '42/abc.webp'))
})

Split the filename-pattern assertion into its own it() to satisfy the "5 tests" AC exactly and make failures immediately diagnosable:

it('returns filename matching {artikelId}/{uuid}.webp pattern', async () => {
  const filename = await processAndSavePhoto(TINY_JPEG, 42)
  expect(filename).toMatch(/^42\/[0-9a-f-]{36}\.webp$/)
})

The uploads route plan uses !target.startsWith(base) without appending path.sep. Per the project's own security conventions (see Secure Code section in the developer persona), it must be !target.startsWith(base + sep) to prevent /uploads-evil/ matching /uploads. Fix before committing.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - The plan's `photos.ts` uses `sharp(...).toFile(dest)` which is correct, but note the plan also shows `.toBuffer()` in the persona guide example — the `toFile` variant is the right choice here since we're writing to disk anyway (avoids a double-buffer). - `uploadDir()` is a private helper that reads `process.env.UPLOAD_DIR ?? 'uploads'` at call time — this is the right approach for test isolation (tests set `process.env.UPLOAD_DIR = TEST_DIR` in `beforeEach`). If the function were called at module-load time the test setup wouldn't work. - The test fixture uses `tmpdir() + Date.now()` for isolation — good. But the `processAndSavePhoto` tests call the real `sharp`, which requires a valid image buffer. The `TINY_JPEG` base64 constant is correct for this, but it creates a real filesystem dependency. This is intentional and acceptable �� `photos.ts` is inherently side-effectful. - The plan currently has **4 tests explicitly** (saves WebP, delete removes file, delete tolerates missing, deleteArtikelPhotos removes dir, deleteArtikelPhotos tolerates missing) but the issue acceptance criteria says **5 tests pass** — counting carefully: saves WebP + correct filename pattern is one test in the plan but the AC counts them separately. The "correct filename pattern" is covered implicitly by the filename regex assertion in the save test. Confirm whether the AC requires a dedicated second `it()` for the filename pattern. - `deletePhoto` silently catches **all** errors, not just `ENOENT`. If sharp writes a corrupted file that causes a permissions error, it will be silently swallowed. The AC says "does not throw if file is already gone" — a tighter catch would be `if (e.code !== 'ENOENT') throw e`. - The `resolveUploadPath` function is in `photos.ts` but has no test. The AC lists it — a one-liner test covering the return value would close this gap. ### Recommendations - Tighten the `deletePhoto` catch to re-throw non-`ENOENT` errors: ```typescript } catch (e: unknown) { if ((e as NodeJS.ErrnoException).code !== 'ENOENT') throw e } ``` - Add a minimal test for `resolveUploadPath`: ```typescript it('resolveUploadPath returns absolute path for filename', () => { const abs = resolveUploadPath('42/abc.webp') expect(abs).toBe(join(TEST_DIR, '42/abc.webp')) }) ``` - Split the filename-pattern assertion into its own `it()` to satisfy the "5 tests" AC exactly and make failures immediately diagnosable: ```typescript it('returns filename matching {artikelId}/{uuid}.webp pattern', async () => { const filename = await processAndSavePhoto(TINY_JPEG, 42) expect(filename).toMatch(/^42\/[0-9a-f-]{36}\.webp$/) }) ``` - The uploads route plan uses `!target.startsWith(base)` without appending `path.sep`. Per the project's own security conventions (see `Secure Code` section in the developer persona), it must be `!target.startsWith(base + sep)` to prevent `/uploads-evil/` matching `/uploads`. Fix before committing.

marcel referenced this issue

2026-05-05 11:26:40 +02:00

Build Admin Inventar — article list and delete #9

marcel commented

2026-05-05 11:26:49 +02:00

🏛️ Markus Keller — Application Architect

Observations

The three-file scope (src/lib/photos.ts, src/lib/photos.test.ts, src/routes/uploads/[...path]/+server.ts) is exactly right — photos.ts is a pure I/O module with one responsibility, the route is a thin streaming adapter. This matches the lib/ one-responsibility-per-module principle.
uploadDir() reads from process.env.UPLOAD_DIR at call time rather than module load — this is the correct design for testability. Do not move it to module scope.
The resolveUploadPath function creates a clean seam between photos.ts (which knows the storage layout) and the route (which calls resolveUploadPath to get the absolute path). This is the right abstraction — the route should not reconstruct the path itself.
The uploads route in the plan uses createReadStream + casts to ReadableStream. In Node 18+ / SvelteKit's Node adapter this works, but it is fragile — SvelteKit's Response expects a Web Streams ReadableStream, not a Node Readable. The cast (stream as unknown as ReadableStream) papers over a type mismatch that will produce runtime errors on certain Node versions.
The plan's route does not set Content-Length. For a 1-year immutable cache this is a cosmetic issue on first load but a correctness issue if the client needs to show a progress bar. Not a blocker for MVP.
deleteArtikelPhotos uses existsSync (sync) to guard before async rm. The guard is redundant — rm(dir, { recursive: true, force: true }) (with force: true) never throws if the directory is missing, and removes the sync I/O call.

Recommendations

Replace the createReadStream cast with a proper Node-to-Web stream conversion:

import { Readable } from 'stream'
const nodeStream = createReadStream(target)
const webStream = Readable.toWeb(nodeStream) as ReadableStream
return new Response(webStream, { headers: { ... } })

Readable.toWeb is available in Node 17+ and is type-safe.

Simplify deleteArtikelPhotos by removing the sync guard:

export async function deleteArtikelPhotos(artikelId: number): Promise<void> {
  await rm(join(uploadDir(), String(artikelId)), { recursive: true, force: true })
}

force: true makes it idempotent. Fewer lines, no sync I/O, same behavior.

Confirm UPLOAD_DIR is set in docker-compose.yml to /app/uploads (it is, per the system design) and that the named volume uploads: maps to that path. The route and the photo library both depend on this env var at runtime — no startup check exists yet. Consider adding a startup guard alongside SESSION_SECRET.

## 🏛️ Markus Keller — Application Architect ### Observations - The three-file scope (`src/lib/photos.ts`, `src/lib/photos.test.ts`, `src/routes/uploads/[...path]/+server.ts`) is exactly right — `photos.ts` is a pure I/O module with one responsibility, the route is a thin streaming adapter. This matches the `lib/` one-responsibility-per-module principle. - `uploadDir()` reads from `process.env.UPLOAD_DIR` at call time rather than module load — this is the correct design for testability. Do not move it to module scope. - The `resolveUploadPath` function creates a clean seam between `photos.ts` (which knows the storage layout) and the route (which calls `resolveUploadPath` to get the absolute path). This is the right abstraction — the route should not reconstruct the path itself. - The uploads route in the plan uses `createReadStream` + casts to `ReadableStream`. In Node 18+ / SvelteKit's Node adapter this works, but it is fragile — SvelteKit's `Response` expects a Web Streams `ReadableStream`, not a Node `Readable`. The cast (`stream as unknown as ReadableStream`) papers over a type mismatch that will produce runtime errors on certain Node versions. - The plan's route does not set `Content-Length`. For a 1-year immutable cache this is a cosmetic issue on first load but a correctness issue if the client needs to show a progress bar. Not a blocker for MVP. - `deleteArtikelPhotos` uses `existsSync` (sync) to guard before async `rm`. The guard is redundant — `rm(dir, { recursive: true, force: true })` (with `force: true`) never throws if the directory is missing, and removes the sync I/O call. ### Recommendations - Replace the `createReadStream` cast with a proper Node-to-Web stream conversion: ```typescript import { Readable } from 'stream' const nodeStream = createReadStream(target) const webStream = Readable.toWeb(nodeStream) as ReadableStream return new Response(webStream, { headers: { ... } }) ``` `Readable.toWeb` is available in Node 17+ and is type-safe. - Simplify `deleteArtikelPhotos` by removing the sync guard: ```typescript export async function deleteArtikelPhotos(artikelId: number): Promise<void> { await rm(join(uploadDir(), String(artikelId)), { recursive: true, force: true }) } ``` `force: true` makes it idempotent. Fewer lines, no sync I/O, same behavior. - Confirm `UPLOAD_DIR` is set in `docker-compose.yml` to `/app/uploads` (it is, per the system design) and that the named volume `uploads:` maps to that path. The route and the photo library both depend on this env var at runtime — no startup check exists yet. Consider adding a startup guard alongside `SESSION_SECRET`.

marcel commented

2026-05-05 11:27:11 +02:00

🔒 Nora Steiner (NullX) — Application Security Engineer

Observations

The issue explicitly carries a security decision: no auth on /uploads/ because URLs are UUID-based and not guessable. I'll audit that decision and everything else on the attack surface.

Path traversal (CWE-22) — the primary risk in this task:

The plan's route has:

if (!target.startsWith(base) || !existsSync(target)) error(404)

Two problems:

base is computed from uploadDir() which may or may not have a trailing separator. startsWith('/app/uploads') would incorrectly allow /app/uploads-evil/.... The fix documented in the project's own persona files is target.startsWith(base + path.sep). This is not applied in the plan.
The error code on path-traversal is 404, not 400. Returning 404 for a traversal attempt is actually better from an information-disclosure standpoint (doesn't confirm the check exists), but the existing security comment in the project (error(400, 'Invalid path')) uses 400. Pick one and be consistent — I have no strong preference, but 400 is more semantically honest that the request was malformed.

No auth on uploads — threat model assessment:

The spec decision (UUID-based, unguessable) is sound for a private family app. UUIDs from crypto.randomUUID() provide 122 bits of entropy. The risk is acceptable if:

UUIDs are never logged in access logs accessible to unauthorized parties (Caddy access log will contain full paths — ensure log access is restricted on the host).
Photos are never served at predictable paths (they won't be, since artikelId/uuid.webp is the pattern — artikelId is sequential but uuid is random).

Content-Type hardcoding:

The route always returns Content-Type: image/webp. If a non-WebP file were written to the uploads directory by some future path, it would be served with the wrong MIME type. This is currently a non-issue because photos.ts is the only writer and always produces .webp. Document this assumption with a comment in the route.

Missing security comment in the route:

The path traversal guard has no explanatory comment. Per project standards, security code must explain the threat model.

Recommendations

Fix the path traversal guard to use path.sep:

import { join, resolve, sep } from 'path'
const UPLOAD_ROOT = resolve(uploadDir())
const target = resolve(join(UPLOAD_ROOT, params.path))
// Prevent path traversal: ensure requested file is within UPLOAD_DIR
if (!target.startsWith(UPLOAD_ROOT + sep)) error(400, 'Invalid path')
if (!existsSync(target)) error(404)

Separate the traversal check (400) from the missing-file check (404) — they are different conditions with different meanings.

Add a security comment above the guard explaining the threat model (path traversal) — one line is enough.

Add a Vitest test for path traversal prevention (this is required per project security standards):

it('resolves path traversal to outside UPLOAD_DIR and throws', async () => {
  // Confirm resolveUploadPath returns a path, but the route must reject it
  // Test the guard logic directly rather than via HTTP
  const base = resolve(TEST_DIR)
  const evil = resolve(join(base, '../../etc/passwd'))
  expect(evil.startsWith(base + sep)).toBe(false)
})

Add UPLOAD_DIR to the startup env-var check alongside SESSION_SECRET — failing silently (falling back to 'uploads' relative path) in production would write files to the wrong location.

## 🔒 Nora Steiner (NullX) — Application Security Engineer ### Observations The issue explicitly carries a security decision: **no auth on `/uploads/`** because URLs are UUID-based and not guessable. I'll audit that decision and everything else on the attack surface. **Path traversal (CWE-22) — the primary risk in this task:** The plan's route has: ```typescript if (!target.startsWith(base) || !existsSync(target)) error(404) ``` Two problems: 1. `base` is computed from `uploadDir()` which may or may not have a trailing separator. `startsWith('/app/uploads')` would incorrectly allow `/app/uploads-evil/...`. The fix documented in the project's own persona files is `target.startsWith(base + path.sep)`. This is not applied in the plan. 2. The error code on path-traversal is `404`, not `400`. Returning `404` for a traversal attempt is actually _better_ from an information-disclosure standpoint (doesn't confirm the check exists), but the existing security comment in the project (`error(400, 'Invalid path')`) uses `400`. Pick one and be consistent — I have no strong preference, but `400` is more semantically honest that the request was malformed. **No auth on uploads — threat model assessment:** The spec decision (UUID-based, unguessable) is sound for a private family app. UUIDs from `crypto.randomUUID()` provide 122 bits of entropy. The risk is acceptable if: - UUIDs are never logged in access logs accessible to unauthorized parties (Caddy access log will contain full paths — ensure log access is restricted on the host). - Photos are never served at predictable paths (they won't be, since `artikelId/uuid.webp` is the pattern — `artikelId` is sequential but `uuid` is random). **Content-Type hardcoding:** The route always returns `Content-Type: image/webp`. If a non-WebP file were written to the uploads directory by some future path, it would be served with the wrong MIME type. This is currently a non-issue because `photos.ts` is the only writer and always produces `.webp`. Document this assumption with a comment in the route. **Missing security comment in the route:** The path traversal guard has no explanatory comment. Per project standards, security code must explain the threat model. ### Recommendations - Fix the path traversal guard to use `path.sep`: ```typescript import { join, resolve, sep } from 'path' const UPLOAD_ROOT = resolve(uploadDir()) const target = resolve(join(UPLOAD_ROOT, params.path)) // Prevent path traversal: ensure requested file is within UPLOAD_DIR if (!target.startsWith(UPLOAD_ROOT + sep)) error(400, 'Invalid path') if (!existsSync(target)) error(404) ``` Separate the traversal check (400) from the missing-file check (404) — they are different conditions with different meanings. - Add a security comment above the guard explaining the threat model (path traversal) — one line is enough. - Add a Vitest test for path traversal prevention (this is required per project security standards): ```typescript it('resolves path traversal to outside UPLOAD_DIR and throws', async () => { // Confirm resolveUploadPath returns a path, but the route must reject it // Test the guard logic directly rather than via HTTP const base = resolve(TEST_DIR) const evil = resolve(join(base, '../../etc/passwd')) expect(evil.startsWith(base + sep)).toBe(false) }) ``` - Add `UPLOAD_DIR` to the startup env-var check alongside `SESSION_SECRET` — failing silently (falling back to `'uploads'` relative path) in production would write files to the wrong location.

marcel commented

2026-05-05 11:27:31 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Observations

The issue specifies 5 Vitest tests by name. The plan's photos.test.ts currently covers exactly 5 behaviors across 4 it() blocks — the "saves WebP" test also asserts the filename pattern, which conflates two assertions. Here's the count:

AC behavior	Plan coverage	Gap?
Saves WebP file	`it('saves a WebP file under UPLOAD_DIR...')`	Covered
Correct filename pattern	Same test, second `expect`	Implicit — not a named test
Delete removes file	`it('removes the file')`	Covered
Delete tolerates missing file	`it('does not throw if file is already gone')`	Covered
deleteArtikelPhotos removes dir	`it('removes the entire artikel directory')`	Covered

The AC states 5 tests pass — strictly interpreted, splitting the filename pattern into its own it() would satisfy "5 named tests" unambiguously.

Positive observations:

beforeEach/afterEach with a unique TEST_DIR per test run — clean, deterministic, no shared state.
Tests use existsSync to assert filesystem side-effects — that's the right assertion for file operations.
TINY_JPEG constant is well-chosen: a real parseable JPEG (~600 bytes) means sharp will run its full pipeline without mocking.

Gaps I notice:

resolveUploadPath has no test — it's listed in the AC as a deliverable function.
No test verifies the Content-Type or Cache-Control headers of the streaming route. The AC requires these headers specifically. These need at minimum a note explaining how they'll be verified (integration test, Playwright, or manual smoke test).
The plan has no test for deleteArtikelPhotos idempotency (second call on already-deleted dir) — the AC says "does not throw if missing." The plan has one test for this (deleteArtikelPhotos does not throw if directory does not exist) which is correct, but it's counted as the 5th test — once the filename-pattern it() is split out there will be 6 tests total, which is fine.
Tests are written as describe blocks — good for organization. The beforeEach/afterEach are at module scope and apply to all tests, which is correct.

Recommendations

Add the resolveUploadPath test as a 6th test (the AC names it, so it needs coverage regardless of the count):

it('resolveUploadPath returns the absolute path for the filename', () => {
  const result = resolveUploadPath('42/test.webp')
  expect(result).toBe(join(TEST_DIR, '42/test.webp'))
})

Verify Content-Type: image/webp and Cache-Control: public, max-age=31536000, immutable in a post-implementation smoke test. These are AC items — they need evidence of being tested. A single fetch('/uploads/...') check in a Playwright smoke test or a dedicated +server.ts unit test would suffice.
Confirm test naming reads as sentences — the current names are good. Keep them: it('saves a WebP file under UPLOAD_DIR/{artikelId}/'), it('removes the file'), etc.
Use describe.each if the deletePhoto / deleteArtikelPhotos idempotency pattern is reused — not needed now but the pattern is worth noting for when auth.test.ts and db.test.ts follow.

## 🧪 Sara Holt — QA Engineer & Test Strategist ### Observations The issue specifies **5 Vitest tests** by name. The plan's `photos.test.ts` currently covers exactly 5 behaviors across 4 `it()` blocks — the "saves WebP" test also asserts the filename pattern, which conflates two assertions. Here's the count: | AC behavior | Plan coverage | Gap? | |---|---|---| | Saves WebP file | `it('saves a WebP file under UPLOAD_DIR...')` | Covered | | Correct filename pattern | Same test, second `expect` | Implicit — not a named test | | Delete removes file | `it('removes the file')` | Covered | | Delete tolerates missing file | `it('does not throw if file is already gone')` | Covered | | deleteArtikelPhotos removes dir | `it('removes the entire artikel directory')` | Covered | The AC states 5 tests pass — strictly interpreted, splitting the filename pattern into its own `it()` would satisfy "5 named tests" unambiguously. **Positive observations:** - `beforeEach`/`afterEach` with a unique `TEST_DIR` per test run — clean, deterministic, no shared state. - Tests use `existsSync` to assert filesystem side-effects — that's the right assertion for file operations. - `TINY_JPEG` constant is well-chosen: a real parseable JPEG (~600 bytes) means sharp will run its full pipeline without mocking. **Gaps I notice:** - `resolveUploadPath` has no test — it's listed in the AC as a deliverable function. - No test verifies the `Content-Type` or `Cache-Control` headers of the streaming route. The AC requires these headers specifically. These need at minimum a note explaining how they'll be verified (integration test, Playwright, or manual smoke test). - The plan has no test for `deleteArtikelPhotos` idempotency (second call on already-deleted dir) — the AC says "does not throw if missing." The plan has one test for this (`deleteArtikelPhotos does not throw if directory does not exist`) which is correct, but it's counted as the 5th test — once the filename-pattern `it()` is split out there will be 6 tests total, which is fine. - Tests are written as `describe` blocks — good for organization. The `beforeEach`/`afterEach` are at module scope and apply to all tests, which is correct. ### Recommendations - Add the `resolveUploadPath` test as a 6th test (the AC names it, so it needs coverage regardless of the count): ```typescript it('resolveUploadPath returns the absolute path for the filename', () => { const result = resolveUploadPath('42/test.webp') expect(result).toBe(join(TEST_DIR, '42/test.webp')) }) ``` - Verify `Content-Type: image/webp` and `Cache-Control: public, max-age=31536000, immutable` in a post-implementation smoke test. These are AC items — they need evidence of being tested. A single `fetch('/uploads/...')` check in a Playwright smoke test or a dedicated `+server.ts` unit test would suffice. - Confirm test naming reads as sentences — the current names are good. Keep them: `it('saves a WebP file under UPLOAD_DIR/{artikelId}/')`, `it('removes the file')`, etc. - Use `describe.each` if the `deletePhoto` / `deleteArtikelPhotos` idempotency pattern is reused — not needed now but the pattern is worth noting for when `auth.test.ts` and `db.test.ts` follow.

marcel commented

2026-05-05 11:27:45 +02:00

🎨 Leonie Voss — UX Design Lead & Accessibility Strategist

Observations

This task is backend-only (photos.ts, photos.test.ts, and the uploads streaming route). No Svelte components, no UI, no design tokens involved. My primary concern is therefore the downstream UX contract this module establishes for the gallery and admin views that depend on it.

What this task determines for UX:

processAndSavePhoto outputs {artikelId}/{uuid}.webp — this relative path is what gets stored in artikel_fotos.filename. The gallery (GalerieKarte) and admin (FotoStreifen) will construct <img src="/uploads/{filename}"> from this. The path format is stable and correct.
The 1200×1200px max / WebP quality 80 constraint will produce files that load fast on mobile (NFR-PERF-002: ≤~800 KB). This is the correct spec decision — no UI concern.
The Cache-Control: public, max-age=31536000, immutable header is excellent for gallery performance: photos won't re-download on navigation. Since filenames are UUID-based, cache busting is automatic on re-upload.

One UX-adjacent concern:

The route returns 404 for both path-traversal attempts and genuinely missing files (in the current plan). When <img src="/uploads/..."> fails, the browser shows a broken image. There is no alt text contract established here. Whoever implements GalerieKarte must ensure every <img> has a meaningful alt attribute — e.g., alt={artikel.titel ?? artikel.kategorie}. This task doesn't implement the <img> tag, but the broken-image fallback behaviour should be addressed before the gallery task.

No accessibility concerns in this task itself — it's a server module with no rendered output.

Recommendations

No design or accessibility changes required in this task's scope.
Add a note in the task commit or in photos.ts JSDoc that the returned filename format ({artikelId}/{uuid}.webp) is the storage contract used by GalerieKarte and FotoStreifen — so future refactoring of the path format knows to update the gallery components too.
When implementing the gallery (Task N+), ensure every <img src="/uploads/{filename}"> carries alt={artikel.titel ?? artikel.kategorie} — broken images need a text fallback for screen readers and slow connections.

## 🎨 Leonie Voss — UX Design Lead & Accessibility Strategist ### Observations This task is backend-only (`photos.ts`, `photos.test.ts`, and the uploads streaming route). No Svelte components, no UI, no design tokens involved. My primary concern is therefore the **downstream UX contract** this module establishes for the gallery and admin views that depend on it. **What this task determines for UX:** - `processAndSavePhoto` outputs `{artikelId}/{uuid}.webp` — this relative path is what gets stored in `artikel_fotos.filename`. The gallery (`GalerieKarte`) and admin (`FotoStreifen`) will construct `<img src="/uploads/{filename}">` from this. The path format is stable and correct. - The 1200×1200px max / WebP quality 80 constraint will produce files that load fast on mobile (NFR-PERF-002: ≤~800 KB). This is the correct spec decision — no UI concern. - The `Cache-Control: public, max-age=31536000, immutable` header is excellent for gallery performance: photos won't re-download on navigation. Since filenames are UUID-based, cache busting is automatic on re-upload. **One UX-adjacent concern:** The route returns `404` for both path-traversal attempts and genuinely missing files (in the current plan). When `<img src="/uploads/...">` fails, the browser shows a broken image. There is no alt text contract established here. Whoever implements `GalerieKarte` must ensure every `<img>` has a meaningful `alt` attribute — e.g., `alt={artikel.titel ?? artikel.kategorie}`. This task doesn't implement the `<img>` tag, but the broken-image fallback behaviour should be addressed before the gallery task. **No accessibility concerns in this task itself** — it's a server module with no rendered output. ### Recommendations - No design or accessibility changes required in this task's scope. - Add a note in the task commit or in `photos.ts` JSDoc that the returned `filename` format (`{artikelId}/{uuid}.webp`) is the storage contract used by `GalerieKarte` and `FotoStreifen` — so future refactoring of the path format knows to update the gallery components too. - When implementing the gallery (Task N+), ensure every `<img src="/uploads/{filename}">` carries `alt={artikel.titel ?? artikel.kategorie}` — broken images need a text fallback for screen readers and slow connections.

marcel commented

2026-05-05 11:28:02 +02:00

🚀 Tobias Wendt — DevOps & Platform Engineer

Observations

UPLOAD_DIR env var: The system design and docker-compose.yml set UPLOAD_DIR: /app/uploads, backed by the named volume uploads: → /app/uploads. This task's photos.ts reads process.env.UPLOAD_DIR ?? 'uploads'. The fallback 'uploads' is a relative path that resolves to the project working directory — in the production container that is /app/uploads only if Node is started from /app, which the Dockerfile does (WORKDIR /app). So the fallback accidentally works in Docker, but it is still wrong: a missing UPLOAD_DIR in production should fail loudly, not silently write to a relative path inside the build output.
Volume persistence: The uploads named volume survives docker compose up --build — any photos processed by this library will persist across container rebuilds. This is correct and already designed into the docker-compose.yml.
Health check: The existing health check pings /health. The uploads route is not /health, so photo streaming failures won't be caught by the health check — this is acceptable; the health check tests liveness, not functionality.
No new infrastructure required for this task. sharp is a runtime dependency that needs the libvips native library — in the node:22-alpine Docker image, sharp ships its own prebuilt binaries for linux/x64 and linux/arm64. No extra apk add needed.
Log hygiene: The route currently logs nothing. A single structured log line on 404 (with the requested path, not the full resolved path) would help diagnose misconfigured UPLOAD_DIR in production without leaking filesystem layout.

Recommendations

Add UPLOAD_DIR to the startup env-var guard in hooks.server.ts (or wherever SESSION_SECRET is checked):
```
if (!process.env.UPLOAD_DIR) {
  throw new Error('UPLOAD_DIR environment variable is required')
}
```
Remove the ?? 'uploads' fallback from photos.ts once the startup check is in place.

Add UPLOAD_DIR to .env.example:

UPLOAD_DIR=/app/uploads   # absolute path; maps to the 'uploads' named Docker volume

Verify the multi-stage Dockerfile copies only the build output, not node_modules from the builder stage — sharp's native binaries must come from npm ci --omit=dev in the runner stage (they are platform-specific). The plan's Dockerfile already does this correctly; just confirm it when implementing.

After Task 4 is merged, add a one-line smoke test to the post-deploy checklist:

# After deploy: upload a photo via admin, then verify the URL streams correctly
curl -fsS https://erbstuecke.raddatz.cloud/uploads/<artikelId>/<uuid>.webp -o /dev/null && echo "Uploads: OK"

## 🚀 Tobias Wendt — DevOps & Platform Engineer ### Observations - **`UPLOAD_DIR` env var:** The system design and `docker-compose.yml` set `UPLOAD_DIR: /app/uploads`, backed by the named volume `uploads: → /app/uploads`. This task's `photos.ts` reads `process.env.UPLOAD_DIR ?? 'uploads'`. The fallback `'uploads'` is a relative path that resolves to the project working directory — in the production container that is `/app/uploads` only if Node is started from `/app`, which the Dockerfile does (`WORKDIR /app`). So the fallback accidentally works in Docker, but it is still wrong: a missing `UPLOAD_DIR` in production should fail loudly, not silently write to a relative path inside the build output. - **Volume persistence:** The `uploads` named volume survives `docker compose up --build` — any photos processed by this library will persist across container rebuilds. This is correct and already designed into the `docker-compose.yml`. - **Health check:** The existing health check pings `/health`. The uploads route is not `/health`, so photo streaming failures won't be caught by the health check — this is acceptable; the health check tests liveness, not functionality. - **No new infrastructure required** for this task. `sharp` is a runtime dependency that needs the `libvips` native library — in the `node:22-alpine` Docker image, sharp ships its own prebuilt binaries for `linux/x64` and `linux/arm64`. No extra `apk add` needed. - **Log hygiene:** The route currently logs nothing. A single structured log line on 404 (with the requested path, not the full resolved path) would help diagnose misconfigured `UPLOAD_DIR` in production without leaking filesystem layout. ### Recommendations - Add `UPLOAD_DIR` to the startup env-var guard in `hooks.server.ts` (or wherever `SESSION_SECRET` is checked): ```typescript if (!process.env.UPLOAD_DIR) { throw new Error('UPLOAD_DIR environment variable is required') } ``` Remove the `?? 'uploads'` fallback from `photos.ts` once the startup check is in place. - Add `UPLOAD_DIR` to `.env.example`: ```bash UPLOAD_DIR=/app/uploads # absolute path; maps to the 'uploads' named Docker volume ``` - Verify the multi-stage Dockerfile copies only the build output, not `node_modules` from the builder stage — `sharp`'s native binaries must come from `npm ci --omit=dev` in the runner stage (they are platform-specific). The plan's Dockerfile already does this correctly; just confirm it when implementing. - After Task 4 is merged, add a one-line smoke test to the post-deploy checklist: ```bash # After deploy: upload a photo via admin, then verify the URL streams correctly curl -fsS https://erbstuecke.raddatz.cloud/uploads/<artikelId>/<uuid>.webp -o /dev/null && echo "Uploads: OK" ```

marcel commented

2026-05-05 11:28:19 +02:00

📋 Elicit — Requirements Engineer

Observations

Issue #4 is well-structured and implementation-ready. Reviewing against the Definition of Ready:

Criterion	Status
Clear, descriptive title	✅
Enough context	✅
Acceptance criteria	✅ (7 items, specific and verifiable)
Labeled/categorized	⚠️ No labels set
Sized	✅ Size: S
Dependencies identified	✅ Depends on #1
Milestone assigned	✅ v1.0 — MVP
Free of ambiguous language	✅

AC completeness check:

Going through each acceptance criterion:

processAndSavePhoto(buffer, artikelId) — ✅ fully specified (dimensions, format, quality, path pattern, return value)
deletePhoto(filename) — ✅ idempotency condition explicit
deleteArtikelPhotos(artikelId) — ✅ idempotency condition explicit
resolveUploadPath(filename) — ✅ return contract specified
GET /uploads/[...path] — ✅ Content-Type and Cache-Control headers named
Path traversal blocked — ✅ specific: "resolved path must start with UPLOAD_DIR"
5 Vitest tests pass — ⚠️ count is ambiguous (see Felix's note — the plan's test file has 4 it() blocks covering 5 behaviors)

One requirements gap:

The AC says path traversal "is blocked" but does not specify what HTTP status is returned. The plan uses error(404), while project security conventions use error(400, 'Invalid path'). This is an unresolved inconsistency between the issue AC and the implementation plan. It should be decided before implementation and the AC updated to reflect it.

NFR-PERF-002 traceability:

The issue correctly references NFR-PERF-002 (≤~800 KB). The spec notes "sharp quality 80 + 1200px limit achieves this for typical photos" — this is a probabilistic claim, not a hard guarantee. Very high-resolution photos with complex content can exceed 800 KB even at quality 80 and 1200px. The AC says "must be ≤ ~800 KB" with the tilde indicating an approximation. This is acceptable for a family app; document that edge cases (extremely dense photos) may exceed the target.

Recommendations

Add labels to the issue (e.g., feature, backend, size:S) for backlog health.
Clarify the path traversal HTTP status code in the AC: choose 400 (malformed request) or 404 (not found, less information-disclosing) and state it explicitly.
Update the "5 Vitest tests" AC to list the 5 test names explicitly — this removes the counting ambiguity and gives the implementer an unambiguous definition of done:
1. processAndSavePhoto saves a WebP file under UPLOAD_DIR/{artikelId}/
2. processAndSavePhoto returns a filename matching {artikelId}/{uuid}.webp
3. deletePhoto removes the file
4. deletePhoto does not throw if file is already gone
5. deleteArtikelPhotos removes the entire {artikelId}/ directory
6. deleteArtikelPhotos does not throw if directory does not exist
7. resolveUploadPath returns the absolute path (this makes 7 — worth counting)

## 📋 Elicit — Requirements Engineer ### Observations Issue #4 is well-structured and implementation-ready. Reviewing against the Definition of Ready: | Criterion | Status | |---|---| | Clear, descriptive title | ✅ | | Enough context | ✅ | | Acceptance criteria | ✅ (7 items, specific and verifiable) | | Labeled/categorized | ⚠️ No labels set | | Sized | ✅ Size: S | | Dependencies identified | ✅ Depends on #1 | | Milestone assigned | ✅ v1.0 — MVP | | Free of ambiguous language | ✅ | **AC completeness check:** Going through each acceptance criterion: 1. `processAndSavePhoto(buffer, artikelId)` — ✅ fully specified (dimensions, format, quality, path pattern, return value) 2. `deletePhoto(filename)` — ✅ idempotency condition explicit 3. `deleteArtikelPhotos(artikelId)` — ✅ idempotency condition explicit 4. `resolveUploadPath(filename)` — ✅ return contract specified 5. `GET /uploads/[...path]` — ✅ Content-Type and Cache-Control headers named 6. Path traversal blocked — ✅ specific: "resolved path must start with UPLOAD_DIR" 7. 5 Vitest tests pass — ⚠️ count is ambiguous (see Felix's note — the plan's test file has 4 `it()` blocks covering 5 behaviors) **One requirements gap:** The AC says path traversal "is blocked" but does not specify what HTTP status is returned. The plan uses `error(404)`, while project security conventions use `error(400, 'Invalid path')`. This is an unresolved inconsistency between the issue AC and the implementation plan. It should be decided before implementation and the AC updated to reflect it. **NFR-PERF-002 traceability:** The issue correctly references NFR-PERF-002 (≤~800 KB). The spec notes "sharp quality 80 + 1200px limit achieves this for typical photos" — this is a probabilistic claim, not a hard guarantee. Very high-resolution photos with complex content can exceed 800 KB even at quality 80 and 1200px. The AC says "must be ≤ ~800 KB" with the tilde indicating an approximation. This is acceptable for a family app; document that edge cases (extremely dense photos) may exceed the target. ### Recommendations - Add labels to the issue (e.g., `feature`, `backend`, `size:S`) for backlog health. - Clarify the path traversal HTTP status code in the AC: choose `400` (malformed request) or `404` (not found, less information-disclosing) and state it explicitly. - Update the "5 Vitest tests" AC to list the 5 test names explicitly — this removes the counting ambiguity and gives the implementer an unambiguous definition of done: 1. `processAndSavePhoto` saves a WebP file under `UPLOAD_DIR/{artikelId}/` 2. `processAndSavePhoto` returns a filename matching `{artikelId}/{uuid}.webp` 3. `deletePhoto` removes the file 4. `deletePhoto` does not throw if file is already gone 5. `deleteArtikelPhotos` removes the entire `{artikelId}/` directory 6. `deleteArtikelPhotos` does not throw if directory does not exist 7. `resolveUploadPath` returns the absolute path _(this makes 7 — worth counting)_

marcel commented

2026-05-05 11:28:35 +02:00

🗳️ Decision Queue — Action Required

2 decisions need your input before implementation starts.

Security

Path traversal response code: 400 vs 404 — The plan uses error(404) for a traversal attempt, but project security conventions (persona files) use error(400, 'Invalid path'). 404 leaks less information (attacker can't confirm the guard exists); 400 is semantically honest (the request is malformed). Both are defensible. Pick one and update the AC to name it explicitly. (Raised by: Nora, Elicit)

Infrastructure / Reliability

UPLOAD_DIR env-var fallback: remove it or keep it? — photos.ts currently has process.env.UPLOAD_DIR ?? 'uploads' as a silent fallback. In production Docker this accidentally resolves correctly, but a missing env var should fail loudly. Decision: add UPLOAD_DIR to the startup guard (alongside SESSION_SECRET) and remove the fallback, OR keep the fallback with a console.warn. Tobias and Nora both flag this — the startup-guard approach is recommended. (Raised by: Tobias, Nora)

## 🗳️ Decision Queue — Action Required _2 decisions need your input before implementation starts._ ### Security - **Path traversal response code: `400` vs `404`** — The plan uses `error(404)` for a traversal attempt, but project security conventions (persona files) use `error(400, 'Invalid path')`. `404` leaks less information (attacker can't confirm the guard exists); `400` is semantically honest (the request is malformed). Both are defensible. Pick one and update the AC to name it explicitly. _(Raised by: Nora, Elicit)_ ### Infrastructure / Reliability - **`UPLOAD_DIR` env-var fallback: remove it or keep it?** — `photos.ts` currently has `process.env.UPLOAD_DIR ?? 'uploads'` as a silent fallback. In production Docker this accidentally resolves correctly, but a missing env var should fail loudly. Decision: add `UPLOAD_DIR` to the startup guard (alongside `SESSION_SECRET`) and remove the fallback, OR keep the fallback with a `console.warn`. Tobias and Nora both flag this — the startup-guard approach is recommended. _(Raised by: Tobias, Nora)_

marcel referenced this issue

2026-05-05 11:28:48 +02:00

Build Admin Inventar — article list and delete #9

marcel referenced this issue

2026-05-05 11:29:10 +02:00

Build Admin Inventar — article list and delete #9

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/wannsee-kram#4

Implement photo processing library and uploads streaming route #4

Task 4 — Plan reference: docs/superpowers/plans/2026-05-05-erbstuecke-wannsee.md

Acceptance criteria

Files to create

NFR

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

Recommendations

🏛️ Markus Keller — Application Architect

Observations

Recommendations

🔒 Nora Steiner (NullX) — Application Security Engineer

Observations

Recommendations

🧪 Sara Holt — QA Engineer & Test Strategist

Observations

Recommendations

🎨 Leonie Voss — UX Design Lead & Accessibility Strategist

Observations

Recommendations

🚀 Tobias Wendt — DevOps & Platform Engineer

Observations

Recommendations

📋 Elicit — Requirements Engineer

Observations

Recommendations

🗳️ Decision Queue — Action Required

Security

Infrastructure / Reliability

Task 4 — Plan reference: `docs/superpowers/plans/2026-05-05-erbstuecke-wannsee.md`