feat: S3 upload pipeline for APNG animated cards (#198) #210

Claude · 2026-04-08T15:04:40Z

Claude commented

2026-04-08 15:04:40 +00:00

Closes #198

Summary

Extends the S3 card storage pipeline to handle APNG animated cards.

app/services/card_storage.py

build_apng_s3_key() — constructs S3 key with .apng extension using the same path pattern as PNG: cards/cardset-{csid:03d}/player-{pid}/v{variant}/{card_type}card.apng
upload_apng_to_s3() — uploads raw APNG bytes with ContentType=image/apng and CacheControl=public, max-age=86400 (matching the animated endpoint's own cache header)
upload_variant_apng() — background-safe end-to-end function: reads APNG from disk, builds key, uploads to S3. No DB update (no animated_url column exists yet); logs success/failure, never raises.

app/routers_v2/players.py

Imports upload_variant_apng from card_storage
Adds background_tasks: BackgroundTasks parameter to get_animated_card
After each new render (cache miss), triggers upload_variant_apng as a background task — skipped for ?tier= dev preview overrides (tier is None guard)

Other observations

No animated_url DB column exists on BattingCard/PitchingCard. The background task uploads to S3 but cannot store a URL back to the DB. A follow-up migration adding animated_url would allow the bot/website to reference the CDN URL directly.
The cache miss path always re-uploads on a forced re-render; this is intentional (consistent with the PNG backfill pattern).

Closes #198 ## Summary Extends the S3 card storage pipeline to handle APNG animated cards. **`app/services/card_storage.py`** - `build_apng_s3_key()` — constructs S3 key with `.apng` extension using the same path pattern as PNG: `cards/cardset-{csid:03d}/player-{pid}/v{variant}/{card_type}card.apng` - `upload_apng_to_s3()` — uploads raw APNG bytes with `ContentType=image/apng` and `CacheControl=public, max-age=86400` (matching the animated endpoint's own cache header) - `upload_variant_apng()` — background-safe end-to-end function: reads APNG from disk, builds key, uploads to S3. No DB update (no `animated_url` column exists yet); logs success/failure, never raises. **`app/routers_v2/players.py`** - Imports `upload_variant_apng` from `card_storage` - Adds `background_tasks: BackgroundTasks` parameter to `get_animated_card` - After each new render (cache miss), triggers `upload_variant_apng` as a background task — skipped for `?tier=` dev preview overrides (`tier is None` guard) ## Other observations - No `animated_url` DB column exists on `BattingCard`/`PitchingCard`. The background task uploads to S3 but cannot store a URL back to the DB. A follow-up migration adding `animated_url` would allow the bot/website to reference the CDN URL directly. - The cache miss path always re-uploads on a forced re-render; this is intentional (consistent with the PNG backfill pattern).

Claude added 1 commit 2026-04-08 15:04:42 +00:00

feat: S3 upload pipeline for APNG animated cards (#198 ) b29450e7d6

Extends card_storage.py with build_apng_s3_key, upload_apng_to_s3, and
upload_variant_apng to handle animated card uploads. Wires get_animated_card
to trigger a background S3 upload on each new render (cache miss, non-preview).

Closes #198

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Claude referenced this pull request

2026-04-08 15:04:55 +00:00

feat: S3 upload pipeline for APNG animated cards #198

Claude added the

ai-reviewing

label 2026-04-08 15:15:50 +00:00

Claude reviewed 2026-04-08 15:17:10 +00:00

Claude left a comment

AI Code Review

Files Reviewed

app/services/card_storage.py (modified — new functions: build_apng_s3_key, upload_apng_to_s3, upload_variant_apng)
app/routers_v2/players.py (modified — new import + BackgroundTasks param + background task trigger)

Findings

Correctness

No issues found.

tier is None guard correctly gates S3 upload — dev preview renders (?tier= param) are excluded ✓
Background task is added after the finally: await page.close() block, so it only fires on a successful render (an exception from generate_animated_card propagates past finally before the add_task call) ✓
Cache-hit path (os.path.isfile(cache_path) and tier is None) returns early before the background task is added — no redundant re-uploads on cache hits ✓
BackgroundTasks already imported at players.py:7 ✓
upload_variant_apng is a top-level import (players.py:43) — follows CLAUDE.md lazy-import rule ✓
except Exception + logger.exception in upload_variant_apng is correct for a background task (never raises) ✓
CacheControl=public, max-age=86400 on the S3 object matches the animated endpoint's own Cache-Control header — intentional and consistent ✓

Security

No issues found.

S3 key is constructed from typed integer/string parameters, not raw user input ✓
apng_path comes from apng_cache_path() (an internal utility), not from user-controlled request data ✓
No hardcoded credentials; S3_BUCKET and S3_REGION use env var overrides ✓

Style & Conventions

No issues found.

Module docstring updated with all new function signatures ✓
upload_variant_apng follows the same structure as backfill_variant_image_url — consistent pattern ✓
Naming is clear and consistent with the existing service layer ✓

Suggestions

Future follow-up: When the animated_url DB column is added, upload_variant_apng will need to be extended to call build_s3_url and persist the URL (mirroring the backfill_variant_image_url pattern). Consider adding a # TODO: update animated_url once column exists comment inside upload_variant_apng to make the gap explicit for the follow-up PR.
No tests added — consistent with the codebase pattern (follow-up test PR expected). Non-blocking.
build_apng_s3_key docstring describes variant as 1-4 = refractor tiers, omitting 0 = base. Technically correct since the endpoint guards T3/T4 only, but the existing build_s3_key docstring includes 0 = base for completeness. Cosmetic.

Verdict: COMMENT

No correctness, security, or convention issues. Implementation is clean and follows the backfill_variant_image_url pattern faithfully. The self-review restriction prevents APPROVED — this is ready to merge.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/services/card_storage.py` (modified — new functions: `build_apng_s3_key`, `upload_apng_to_s3`, `upload_variant_apng`) - `app/routers_v2/players.py` (modified — new import + `BackgroundTasks` param + background task trigger) ### Findings #### Correctness No issues found. - `tier is None` guard correctly gates S3 upload — dev preview renders (`?tier=` param) are excluded ✓ - Background task is added after the `finally: await page.close()` block, so it only fires on a successful render (an exception from `generate_animated_card` propagates past `finally` before the `add_task` call) ✓ - Cache-hit path (`os.path.isfile(cache_path) and tier is None`) returns early before the background task is added — no redundant re-uploads on cache hits ✓ - `BackgroundTasks` already imported at `players.py:7` ✓ - `upload_variant_apng` is a top-level import (`players.py:43`) — follows CLAUDE.md lazy-import rule ✓ - `except Exception` + `logger.exception` in `upload_variant_apng` is correct for a background task (never raises) ✓ - `CacheControl=public, max-age=86400` on the S3 object matches the animated endpoint's own `Cache-Control` header — intentional and consistent ✓ #### Security No issues found. - S3 key is constructed from typed integer/string parameters, not raw user input ✓ - `apng_path` comes from `apng_cache_path()` (an internal utility), not from user-controlled request data ✓ - No hardcoded credentials; `S3_BUCKET` and `S3_REGION` use env var overrides ✓ #### Style & Conventions No issues found. - Module docstring updated with all new function signatures ✓ - `upload_variant_apng` follows the same structure as `backfill_variant_image_url` — consistent pattern ✓ - Naming is clear and consistent with the existing service layer ✓ #### Suggestions - **Future follow-up**: When the `animated_url` DB column is added, `upload_variant_apng` will need to be extended to call `build_s3_url` and persist the URL (mirroring the `backfill_variant_image_url` pattern). Consider adding a `# TODO: update animated_url once column exists` comment inside `upload_variant_apng` to make the gap explicit for the follow-up PR. - **No tests added** — consistent with the codebase pattern (follow-up test PR expected). Non-blocking. - `build_apng_s3_key` docstring describes `variant` as `1-4 = refractor tiers`, omitting `0 = base`. Technically correct since the endpoint guards T3/T4 only, but the existing `build_s3_key` docstring includes `0 = base` for completeness. Cosmetic. ### Verdict: COMMENT No correctness, security, or convention issues. Implementation is clean and follows the `backfill_variant_image_url` pattern faithfully. The self-review restriction prevents APPROVED — this is ready to merge. --- *Automated review by Claude PR Reviewer*

Claude added

ai-reviewed

and removed

ai-reviewing

labels 2026-04-08 15:17:49 +00:00

cal approved these changes 2026-04-08 15:25:35 +00:00

Dismissed

cal left a comment

Approved via pd-pr

cal approved these changes 2026-04-08 15:25:39 +00:00

cal left a comment

Approved via pd-pr

cal merged commit d83a4bdbb7 into main

2026-04-08 15:25:41 +00:00

cal deleted branch issue/198-feat-s3-upload-pipeline-for-apng-animated-cards

2026-04-08 15:25:41 +00:00

cal referenced this issue from a commit

2026-04-08 15:25:43 +00:00

Merge pull request 'feat: S3 upload pipeline for APNG animated cards (#198)' (#210) from issue/198-feat-s3-upload-pipeline-for-apng-animated-cards into main

Sign in to join this conversation.

No reviewers

No Milestone

No project

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/paper-dynasty-database#210