fix: return placeholder SVG for missing photo files

forgejo_admin commented

2026-04-07 17:32:33 +00:00

Owner

Summary

When a player's photo_url references a file that no longer exists on disk, ImageStaticFiles now returns a silhouette SVG placeholder instead of a raw 404. This prevents broken-image icons in the browser.

Changes

src/basketball_api/static.py — Override get_response to catch 404 HTTPException and return a lightweight silhouette SVG placeholder with image/svg+xml content type
tests/test_photo_content_type.py — Update test_nonexistent_file_404 to test_nonexistent_file_returns_placeholder, asserting 200 status and SVG content

Test Plan

pytest tests/test_photo_content_type.py — 6/6 pass (including the new placeholder test)
pytest tests/test_upload.py — 77 pass, no regressions
Existing photo serve flow unchanged: real files still served with correct MIME types

Review Checklist

ruff format and ruff check pass
Tests updated for new behavior
No regressions in upload or photo serving tests
Placeholder SVG reuses the same silhouette pattern from tryouts.py

Closes forgejo_admin/basketball-api#374
Board item: #879 on board-westside-basketball

## Summary When a player's `photo_url` references a file that no longer exists on disk, `ImageStaticFiles` now returns a silhouette SVG placeholder instead of a raw 404. This prevents broken-image icons in the browser. ## Changes - `src/basketball_api/static.py` — Override `get_response` to catch 404 `HTTPException` and return a lightweight silhouette SVG placeholder with `image/svg+xml` content type - `tests/test_photo_content_type.py` — Update `test_nonexistent_file_404` to `test_nonexistent_file_returns_placeholder`, asserting 200 status and SVG content ## Test Plan - `pytest tests/test_photo_content_type.py` — 6/6 pass (including the new placeholder test) - `pytest tests/test_upload.py` — 77 pass, no regressions - Existing photo serve flow unchanged: real files still served with correct MIME types ## Review Checklist - [x] ruff format and ruff check pass - [x] Tests updated for new behavior - [x] No regressions in upload or photo serving tests - [x] Placeholder SVG reuses the same silhouette pattern from tryouts.py ## Related Notes - Closes forgejo_admin/basketball-api#374 - Board item: #879 on board-westside-basketball

forgejo_admin added 2 commits

2026-04-07 17:32:33 +00:00

feat: include is_public in admin players list response

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

3c167eae94

The AdminPlayerItem model and GET /admin/players response now include
the is_public boolean field, enabling the westside-app admin UI to
display and toggle player visibility state.

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

fix: return silhouette placeholder SVG for missing photo files instead of 404

ci/woodpecker/pr/woodpecker Pipeline failed

Details

9b4ba04ef0

When a player's photo_url references a file that no longer exists on disk,
ImageStaticFiles now catches the 404 and returns a lightweight silhouette
SVG placeholder. This prevents broken-image icons in the browser.

Closes forgejo_admin/basketball-api#374

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

forgejo_admin force-pushed 374-missing-photo-placeholder from 9b4ba04ef0

ci/woodpecker/pr/woodpecker Pipeline failed

Details

to a7ae05aaff

ci/woodpecker/pr/woodpecker Pipeline failed

Details

2026-04-07 17:33:31 +00:00

Compare

forgejo_admin commented

2026-04-07 17:33:50 +00:00

Author

Owner

QA Review

Scope: 2 files changed, 36 additions, 3 deletions. Scoped correctly to issue #374.

static.py

Approach is sound. Overriding get_response to catch HTTPException(404) and return a placeholder is the minimal, correct interception point. Existing file serving is untouched.
Singleton _PLACEHOLDER_RESPONSE -- Starlette Response objects are safe to reuse across requests since they are immutable after construction. No concurrency issue.
SVG placeholder reuses the same silhouette path data from tryouts.py:195-201, consistent with the codebase pattern.
No silent swallowing -- only 404 is caught; other HTTP exceptions (401, 405) re-raise correctly.

test_photo_content_type.py

Test correctly updated from asserting 404 to asserting 200 + SVG content.
Content-type assertion uses in check, which handles presence/absence of charset parameter correctly.
All 5 existing tests still pass (MIME type correctness for jpg/jpeg/png/webp + no octet-stream).

Nits

None. Clean, minimal change.

Note

Initial push included an unrelated is_public commit from local main. Rebased to origin/main so the PR now contains only the placeholder fix commit.

VERDICT: APPROVED

## QA Review **Scope:** 2 files changed, 36 additions, 3 deletions. Scoped correctly to issue #374. ### static.py - **Approach is sound.** Overriding `get_response` to catch `HTTPException(404)` and return a placeholder is the minimal, correct interception point. Existing file serving is untouched. - **Singleton `_PLACEHOLDER_RESPONSE`** -- Starlette `Response` objects are safe to reuse across requests since they are immutable after construction. No concurrency issue. - **SVG placeholder** reuses the same silhouette path data from `tryouts.py:195-201`, consistent with the codebase pattern. - **No silent swallowing** -- only 404 is caught; other HTTP exceptions (401, 405) re-raise correctly. ### test_photo_content_type.py - Test correctly updated from asserting 404 to asserting 200 + SVG content. - Content-type assertion uses `in` check, which handles presence/absence of `charset` parameter correctly. - All 5 existing tests still pass (MIME type correctness for jpg/jpeg/png/webp + no octet-stream). ### Nits - None. Clean, minimal change. ### Note Initial push included an unrelated `is_public` commit from local main. Rebased to `origin/main` so the PR now contains only the placeholder fix commit. **VERDICT: APPROVED**

forgejo_admin commented

2026-04-07 17:45:58 +00:00

Author

Owner

PR #377 Review

DOMAIN REVIEW

Stack: Python / FastAPI / Starlette / pytest

Correctness

get_response override correctly catches HTTPException with status 404 and returns the placeholder. Non-404 exceptions are re-raised. This is sound.
media_type="image/svg+xml" is correctly set on the placeholder response. Browsers will render it as an image.
The mount point is /uploads/photos only (main.py:101-102), so the fallback is scoped exclusively to photo requests. Non-photo static paths are unaffected.

Performance

_PLACEHOLDER_RESPONSE is a module-level singleton -- allocated once, returned on every 404. No disk reads, no string concatenation per request. Good pattern.
Note: Starlette Response objects are safe to share across requests since the body is set at construction time and headers are regenerated per call by the ASGI interface.

DRY: SVG duplication with tryouts.py

tryouts.py:196-201 defines _SILHOUETTE_SVG with the same <path> data but different SVG attributes (no fill, no width/height).
static.py defines _PLACEHOLDER_SVG with fill="#9e9e9e" width="100%" height="100%" added to the <svg> element.
The path data (M12 12c2.76...) is identical -- this is the Material Design "person" icon. The attribute differences are intentional (tryouts embeds it inline in HTML where CSS controls sizing/color; static.py serves it standalone where the SVG must carry its own styling). This is acceptable divergence, not a DRY blocker.

Existing flow preserved

file_response is untouched. Real files still get the explicit MIME type override. The new get_response sits above file_response in the call chain and only intercepts when the parent raises 404.

Test coverage

test_nonexistent_file_returns_placeholder asserts: status 200, image/svg+xml in content-type, <svg and viewBox present in body. Covers the happy path for the fallback.
All 4 existing MIME-type tests and the octet-stream guard remain unchanged. No regressions.

BLOCKERS

None.

NITS

Extract shared SVG path to a constant: The M12 12c2.76... path data is duplicated between static.py and routes/tryouts.py. Consider extracting the path data (not the full SVG) to a shared constant in a common module. Low priority since the two usages intentionally differ in SVG attributes, but it would prevent future divergence of the path itself.
Test could assert exact content-type: The test checks "image/svg+xml" in response.headers["content-type"] (substring match). An exact match == "image/svg+xml; charset=utf-8" or at minimum response.headers["content-type"].startswith("image/svg+xml") would be marginally more precise, though the current approach is fine in practice.

SOP COMPLIANCE

Branch named after issue: 374-missing-photo-placeholder matches issue #374
PR body follows template: Summary, Changes, Test Plan, Related all present
Related references issue: Closes forgejo_admin/basketball-api#374, board item #879
No secrets committed
No scope creep: 2 files changed, both directly related to the fix
Tests updated for new behavior

PROCESS OBSERVATIONS

Clean, minimal fix. 36 additions / 3 deletions for a well-scoped bug fix.
The singleton response pattern is a good choice -- avoids per-request allocation for what could be a hot path if many players have stale photo URLs.
The SVG duplication nit is worth tracking as discovered scope for a future cleanup ticket but does not block this PR.

VERDICT: APPROVED

## PR #377 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / Starlette / pytest **Correctness** - `get_response` override correctly catches `HTTPException` with status 404 and returns the placeholder. Non-404 exceptions are re-raised. This is sound. - `media_type="image/svg+xml"` is correctly set on the placeholder response. Browsers will render it as an image. - The mount point is `/uploads/photos` only (`main.py:101-102`), so the fallback is scoped exclusively to photo requests. Non-photo static paths are unaffected. **Performance** - `_PLACEHOLDER_RESPONSE` is a module-level singleton -- allocated once, returned on every 404. No disk reads, no string concatenation per request. Good pattern. - Note: Starlette `Response` objects are safe to share across requests since the body is set at construction time and headers are regenerated per call by the ASGI interface. **DRY: SVG duplication with tryouts.py** - `tryouts.py:196-201` defines `_SILHOUETTE_SVG` with the same `<path>` data but different SVG attributes (no `fill`, no `width`/`height`). - `static.py` defines `_PLACEHOLDER_SVG` with `fill="#9e9e9e" width="100%" height="100%"` added to the `<svg>` element. - The path data (`M12 12c2.76...`) is identical -- this is the Material Design "person" icon. The attribute differences are intentional (tryouts embeds it inline in HTML where CSS controls sizing/color; static.py serves it standalone where the SVG must carry its own styling). This is acceptable divergence, not a DRY blocker. **Existing flow preserved** - `file_response` is untouched. Real files still get the explicit MIME type override. The new `get_response` sits above `file_response` in the call chain and only intercepts when the parent raises 404. **Test coverage** - `test_nonexistent_file_returns_placeholder` asserts: status 200, `image/svg+xml` in content-type, `<svg` and `viewBox` present in body. Covers the happy path for the fallback. - All 4 existing MIME-type tests and the octet-stream guard remain unchanged. No regressions. ### BLOCKERS None. ### NITS 1. **Extract shared SVG path to a constant**: The `M12 12c2.76...` path data is duplicated between `static.py` and `routes/tryouts.py`. Consider extracting the path data (not the full SVG) to a shared constant in a common module. Low priority since the two usages intentionally differ in SVG attributes, but it would prevent future divergence of the path itself. 2. **Test could assert exact content-type**: The test checks `"image/svg+xml" in response.headers["content-type"]` (substring match). An exact match `== "image/svg+xml; charset=utf-8"` or at minimum `response.headers["content-type"].startswith("image/svg+xml")` would be marginally more precise, though the current approach is fine in practice. ### SOP COMPLIANCE - [x] Branch named after issue: `374-missing-photo-placeholder` matches issue #374 - [x] PR body follows template: Summary, Changes, Test Plan, Related all present - [x] Related references issue: `Closes forgejo_admin/basketball-api#374`, board item #879 - [x] No secrets committed - [x] No scope creep: 2 files changed, both directly related to the fix - [x] Tests updated for new behavior ### PROCESS OBSERVATIONS - Clean, minimal fix. 36 additions / 3 deletions for a well-scoped bug fix. - The singleton response pattern is a good choice -- avoids per-request allocation for what could be a hot path if many players have stale photo URLs. - The SVG duplication nit is worth tracking as discovered scope for a future cleanup ticket but does not block this PR. ### VERDICT: APPROVED