feat: wire photo upload on player profile edit

forgejo_admin commented

2026-03-18 19:49:23 +00:00

Author

Owner

PR #43 Review

DOMAIN REVIEW

Tech stack: SvelteKit SPA (adapter-static) + vanilla CSS + client-side JS API layer. Domain checklist: SvelteKit/TypeScript/CSS.

API layer (src/lib/api.js)

DRY concern between apiFetch and apiUpload: Both functions duplicate the token-fetch, header-build, fetch-call, error-check, and JSON-parse pattern. The only difference is: (a) apiUpload omits Content-Type, (b) always uses POST, and (c) uses FormData body. This is a minor DRY issue but not in an auth/security path -- both delegate to getToken() the same way, so divergence risk is low. Non-blocking.
API_BASE export is correct. Previously private, now exported so the Svelte component can construct full image URLs. Reasonable change -- the constant was already hardcoded and used by both functions.
Error message in apiUpload differs from apiFetch: apiFetch throws API ${res.status}: ${res.statusText} while apiUpload throws Upload failed: ${res.status} ${res.statusText}. Inconsistent but non-blocking -- the consumer catches and displays a user-friendly message anyway.

Component (src/routes/players/[id]/+page.svelte)

No client-side file validation before upload. The accept="image/*" attribute on the input is advisory only (browser hint). A user could bypass it. There is no check for file size or MIME type before sending to the server. The backend endpoint (POST /upload/photo) should enforce these limits, but a client-side size check (e.g., max 5MB) would improve UX by avoiding a wasted upload + cryptic error. Non-blocking -- server-side validation is the real gate.
Photo URL construction: {API_BASE}{player.photo_url} assumes photo_url is a relative path starting with /. If the backend ever returns a full URL, this would double-prefix. Given the decision to use MinIO (per plan-wkq), the backend currently returns paths like /uploads/photos/filename.jpg, so this works. But it is fragile. Non-blocking.
Duplicated avatar rendering block: The photo-or-initials logic appears twice in the template (lines 262-266 for editable view, lines 274-278 for non-editable view). This is a minor template DRY issue. A Svelte snippet ({#snippet}) could eliminate the duplication. Non-blocking.
Loading state UX is good. The overlay shows ... during upload, the file input is disabled during upload, and the input is reset after completion to allow re-upload of the same file. Error state is displayed and cleared on retry.
Accessibility: The <label for="photo-upload"> wraps the avatar and provides click-to-upload. The <img> tag has a proper alt attribute. The error message uses a visible .photo-error div. However, the error is not announced to screen readers (no role="alert" or aria-live). Non-blocking nit.

CSS (src/app.css)

.profile-avatar-img uses object-fit: cover -- correct for avatar images. Applied alongside existing .profile-avatar class which sets width/height/border-radius, so the image will be properly clipped to a circle. Clean.
.photo-error uses var(--color-red) -- consistent with design token usage elsewhere in the file. No magic numbers.

BLOCKERS

No blockers found.

Test coverage: This repo has zero tests (confirmed: no .test.* or .spec.* files outside node_modules). This is a pre-existing condition tracked in the plan-wkq epilogue (PR #37, item 2: "Zero test coverage -- no unit or integration tests for any SPA route"). The PR is additive frontend wiring of an existing file input to an existing backend endpoint -- it does not introduce new architectural patterns or security-critical logic. The test gap is a repo-wide issue, not specific to this PR.
No secrets committed. API_BASE is a Tailscale hostname, not a secret.
No unvalidated user input risk. The photo file is sent to the backend as FormData; the photo_url returned by the backend is stored and displayed. The player.name used in alt attributes comes from the API, not user input in this flow.
No DRY violation in auth/security paths. Both apiFetch and apiUpload use the same getToken() call.

NITS

Client-side file size check. Add a guard like if (file.size > 5 * 1024 * 1024) { photoError = 'File too large (max 5MB)'; return; } before uploading. Prevents wasted bandwidth on oversized files.
aria-live="polite" on .photo-error for screen reader announcement of upload failures.
Template DRY: The photo-or-initials rendering could use a Svelte {#snippet} to avoid the duplicated {#if player.photo_url} block in the editable and non-editable branches.
getInitials() still duplicated across 7 files (tracked in plan-wkq epilogue, PR #37 item 1). This PR does not make it worse but is a reminder the extraction to $lib/utils.js is still pending.
Inconsistent error message format between apiFetch (API ${status}) and apiUpload (Upload failed: ${status}). Minor -- consider aligning.

SOP COMPLIANCE

Branch named after issue: 42-wire-photo-upload-on-player-profile-edit matches issue #42
PR body has Summary, Changes, Test Plan, Related sections
Related section references plan slug: Missing plan-wkq reference. Only has Closes #42.
No secrets committed
No unnecessary file changes -- 3 files changed, all directly relevant
Commit messages are descriptive

PROCESS OBSERVATIONS

Deployment frequency: Small, focused PR (96 additions, 6 deletions, 3 files). Clean scope. Good for DF.
Change failure risk: Low. This wires existing UI elements to an existing backend endpoint. No new routes, no auth changes, no state management changes.
Test coverage gap: The repo-wide zero-test situation remains the biggest CFR risk. Tracked in epilogue but needs its own issue and phase.
Plan slug missing from Related: The PR body should reference plan-wkq for traceability. This is a process gap, not a code gap.

VERDICT: APPROVED

## PR #43 Review ### DOMAIN REVIEW **Tech stack:** SvelteKit SPA (adapter-static) + vanilla CSS + client-side JS API layer. Domain checklist: SvelteKit/TypeScript/CSS. **API layer (`src/lib/api.js`)** 1. **DRY concern between `apiFetch` and `apiUpload`:** Both functions duplicate the token-fetch, header-build, fetch-call, error-check, and JSON-parse pattern. The only difference is: (a) `apiUpload` omits `Content-Type`, (b) always uses POST, and (c) uses `FormData` body. This is a minor DRY issue but not in an auth/security path -- both delegate to `getToken()` the same way, so divergence risk is low. Non-blocking. 2. **`API_BASE` export is correct.** Previously private, now exported so the Svelte component can construct full image URLs. Reasonable change -- the constant was already hardcoded and used by both functions. 3. **Error message in `apiUpload` differs from `apiFetch`:** `apiFetch` throws `API ${res.status}: ${res.statusText}` while `apiUpload` throws `Upload failed: ${res.status} ${res.statusText}`. Inconsistent but non-blocking -- the consumer catches and displays a user-friendly message anyway. **Component (`src/routes/players/[id]/+page.svelte`)** 4. **No client-side file validation before upload.** The `accept="image/*"` attribute on the input is advisory only (browser hint). A user could bypass it. There is no check for file size or MIME type before sending to the server. The backend endpoint (`POST /upload/photo`) should enforce these limits, but a client-side size check (e.g., max 5MB) would improve UX by avoiding a wasted upload + cryptic error. Non-blocking -- server-side validation is the real gate. 5. **Photo URL construction:** `{API_BASE}{player.photo_url}` assumes `photo_url` is a relative path starting with `/`. If the backend ever returns a full URL, this would double-prefix. Given the decision to use MinIO (per plan-wkq), the backend currently returns paths like `/uploads/photos/filename.jpg`, so this works. But it is fragile. Non-blocking. 6. **Duplicated avatar rendering block:** The photo-or-initials logic appears twice in the template (lines 262-266 for editable view, lines 274-278 for non-editable view). This is a minor template DRY issue. A Svelte snippet (`{#snippet}`) could eliminate the duplication. Non-blocking. 7. **Loading state UX is good.** The overlay shows `...` during upload, the file input is disabled during upload, and the input is reset after completion to allow re-upload of the same file. Error state is displayed and cleared on retry. 8. **Accessibility:** The `<label for="photo-upload">` wraps the avatar and provides click-to-upload. The `<img>` tag has a proper `alt` attribute. The error message uses a visible `.photo-error` div. However, the error is not announced to screen readers (no `role="alert"` or `aria-live`). Non-blocking nit. **CSS (`src/app.css`)** 9. **`.profile-avatar-img` uses `object-fit: cover`** -- correct for avatar images. Applied alongside existing `.profile-avatar` class which sets width/height/border-radius, so the image will be properly clipped to a circle. Clean. 10. **`.photo-error` uses `var(--color-red)`** -- consistent with design token usage elsewhere in the file. No magic numbers. ### BLOCKERS **No blockers found.** - **Test coverage:** This repo has zero tests (confirmed: no `.test.*` or `.spec.*` files outside `node_modules`). This is a pre-existing condition tracked in the plan-wkq epilogue (PR #37, item 2: "Zero test coverage -- no unit or integration tests for any SPA route"). The PR is additive frontend wiring of an existing file input to an existing backend endpoint -- it does not introduce new architectural patterns or security-critical logic. The test gap is a repo-wide issue, not specific to this PR. - **No secrets committed.** `API_BASE` is a Tailscale hostname, not a secret. - **No unvalidated user input risk.** The photo file is sent to the backend as `FormData`; the `photo_url` returned by the backend is stored and displayed. The `player.name` used in `alt` attributes comes from the API, not user input in this flow. - **No DRY violation in auth/security paths.** Both `apiFetch` and `apiUpload` use the same `getToken()` call. ### NITS 1. **Client-side file size check.** Add a guard like `if (file.size > 5 * 1024 * 1024) { photoError = 'File too large (max 5MB)'; return; }` before uploading. Prevents wasted bandwidth on oversized files. 2. **`aria-live="polite"` on `.photo-error`** for screen reader announcement of upload failures. 3. **Template DRY:** The photo-or-initials rendering could use a Svelte `{#snippet}` to avoid the duplicated `{#if player.photo_url}` block in the editable and non-editable branches. 4. **`getInitials()` still duplicated across 7 files** (tracked in plan-wkq epilogue, PR #37 item 1). This PR does not make it worse but is a reminder the extraction to `$lib/utils.js` is still pending. 5. **Inconsistent error message format** between `apiFetch` (`API ${status}`) and `apiUpload` (`Upload failed: ${status}`). Minor -- consider aligning. ### SOP COMPLIANCE - [x] Branch named after issue: `42-wire-photo-upload-on-player-profile-edit` matches issue #42 - [x] PR body has Summary, Changes, Test Plan, Related sections - [ ] Related section references plan slug: Missing `plan-wkq` reference. Only has `Closes #42`. - [x] No secrets committed - [x] No unnecessary file changes -- 3 files changed, all directly relevant - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - **Deployment frequency:** Small, focused PR (96 additions, 6 deletions, 3 files). Clean scope. Good for DF. - **Change failure risk:** Low. This wires existing UI elements to an existing backend endpoint. No new routes, no auth changes, no state management changes. - **Test coverage gap:** The repo-wide zero-test situation remains the biggest CFR risk. Tracked in epilogue but needs its own issue and phase. - **Plan slug missing from Related:** The PR body should reference `plan-wkq` for traceability. This is a process gap, not a code gap. ### VERDICT: APPROVED

2026-03-18 19:47:10 +00:00

Summary

Connects the existing file input on the player profile page to the backend POST /upload/photo endpoint. On successful upload, saves the photo URL to the player record and displays the uploaded photo in the avatar circle immediately.

Changes

src/lib/api.js: Export API_BASE for constructing full photo URLs. Add apiUpload() helper for multipart/form-data uploads (omits Content-Type header so the browser sets the multipart boundary automatically).
src/routes/players/[id]/+page.svelte: Wire onchange handler on the file input that uploads via apiUpload('/upload/photo'), then PUTs the returned photo_url to /players/{id}. Shows an <img> tag with the photo when player.photo_url exists, falling back to initials. Includes loading indicator on the edit overlay and error message on failure.
src/app.css: Add .profile-avatar-img (object-fit: cover for uploaded photos) and .photo-error styles.

Test Plan

Navigate to /players/{id} as an admin or owner
Click the avatar edit icon, select a photo file
Verify loading indicator appears during upload
Verify avatar updates to show the uploaded photo on success
Verify error message appears if upload fails (e.g., network error)
Verify non-editing view also shows the photo when photo_url is set
npm run check passes — no new errors introduced (2 pre-existing type warnings unrelated to this change)

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes #42

## Summary Connects the existing file input on the player profile page to the backend `POST /upload/photo` endpoint. On successful upload, saves the photo URL to the player record and displays the uploaded photo in the avatar circle immediately. ## Changes - `src/lib/api.js`: Export `API_BASE` for constructing full photo URLs. Add `apiUpload()` helper for multipart/form-data uploads (omits `Content-Type` header so the browser sets the multipart boundary automatically). - `src/routes/players/[id]/+page.svelte`: Wire `onchange` handler on the file input that uploads via `apiUpload('/upload/photo')`, then PUTs the returned `photo_url` to `/players/{id}`. Shows an `<img>` tag with the photo when `player.photo_url` exists, falling back to initials. Includes loading indicator on the edit overlay and error message on failure. - `src/app.css`: Add `.profile-avatar-img` (object-fit: cover for uploaded photos) and `.photo-error` styles. ## Test Plan - [ ] Navigate to `/players/{id}` as an admin or owner - [ ] Click the avatar edit icon, select a photo file - [ ] Verify loading indicator appears during upload - [ ] Verify avatar updates to show the uploaded photo on success - [ ] Verify error message appears if upload fails (e.g., network error) - [ ] Verify non-editing view also shows the photo when `photo_url` is set - [ ] `npm run check` passes — no new errors introduced (2 pre-existing type warnings unrelated to this change) ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #42

forgejo_admin added 1 commit

feat: wire photo upload on player profile edit page 56648bff38

Add apiUpload helper in api.js for multipart/form-data uploads (no
Content-Type header so the browser sets the multipart boundary). Export
API_BASE for constructing full photo URLs.

On the player profile page, the file input now calls handlePhotoUpload
which POSTs to /upload/photo, saves the returned photo_url to the player
via PUT /players/{id}, and updates local state so the avatar shows the
uploaded image immediately. Includes loading and error states.

Closes #42

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

2026-03-18 19:47:38 +00:00

Review-fix loop: PASS -- no issues found.

Reviewed all 3 changed files (96 additions, 6 deletions):

src/lib/api.js -- apiUpload correctly omits Content-Type for multipart boundary, follows same auth pattern as apiFetch, proper error handling.
src/routes/players/[id]/+page.svelte -- Upload flow is correct: POST file, PUT photo_url, update local state, reset input, handle errors. Template correctly branches on player.photo_url for both editable and read-only avatar views. Loading indicator on overlay.
src/app.css -- object-fit: cover on .profile-avatar-img works with existing 96x96 circular avatar. Error style is clean.

npm run check passes with no new errors (2 pre-existing type warnings in unrelated files).

**Review-fix loop: PASS -- no issues found.** Reviewed all 3 changed files (96 additions, 6 deletions): - `src/lib/api.js` -- `apiUpload` correctly omits `Content-Type` for multipart boundary, follows same auth pattern as `apiFetch`, proper error handling. - `src/routes/players/[id]/+page.svelte` -- Upload flow is correct: POST file, PUT photo_url, update local state, reset input, handle errors. Template correctly branches on `player.photo_url` for both editable and read-only avatar views. Loading indicator on overlay. - `src/app.css` -- `object-fit: cover` on `.profile-avatar-img` works with existing 96x96 circular avatar. Error style is clean. `npm run check` passes with no new errors (2 pre-existing type warnings in unrelated files).