feat(images): add larger TV image variants - #336
Conversation
|
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:
📝 WalkthroughWalkthroughAdds w780 episode still variants and w1280 logo variants, updates API and catalog path selection, expands image-cache ladders, and adds cached S3 existence checks with fallback presigning for missing new variants. ChangesEpisode still w780 variant support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PluginImageResolver
participant variantExistsCache
participant S3Presigner
PluginImageResolver->>variantExistsCache: look up variant existence
alt cache miss
PluginImageResolver->>S3Presigner: ObjectExists(requested key)
S3Presigner-->>PluginImageResolver: existence result
PluginImageResolver->>variantExistsCache: cache successful result
end
alt requested variant missing
PluginImageResolver->>S3Presigner: PresignGetURL(fallback key)
else requested variant exists or check errors
PluginImageResolver->>S3Presigner: PresignGetURL(requested key)
end
S3Presigner-->>PluginImageResolver: presigned URL
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66f8ab4cc9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
66f8ab4 to
c2f220a
Compare
|
Addressed the fallback caching review in c2f220a. Missing new-variant S3 fallbacks now cap their resolved-URL cache window to the same short missing-variant TTL, so refreshed w780/w1280 objects are picked up promptly instead of waiting on the full presign cache window. Added a regression test for the fallback URL cache TTL and reran the relevant Go package tests. |
c2f220a to
e0b6247
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/metadata/image_resolver_test.go (1)
379-424: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the
existsandObjectExistserror branches.The tests validate the missing-variant fallback and its TTL well, but
existingVariantKeyhas two untested branches: object-exists (presignw780directly, cachetruefor 24h) and the optimistic error path (presign requested key, no caching). Adding cases viaexisting[...]=trueand anObjectExistserror would lock in the no-fallback behavior and prevent regressions.🤖 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 `@internal/metadata/image_resolver_test.go` around lines 379 - 424, Add tests in TestPluginImageResolverS3... to cover the remaining existingVariantKey branches: one where fakeS3ImagePresigner reports ObjectExists true for the requested w780 key so ResolveImageURLWithExpiry returns the direct w780 presign and caches it with the existing-variant TTL, and another where ObjectExists returns an error so the resolver still presigns the requested key but does not cache the existence result. Use the existing PluginImageResolver, SetS3Presigner, and fakeS3ImagePresigner helpers to assert both the returned URL and the presigner call counts/keys.
🤖 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 `@internal/metadata/image_resolver_test.go`:
- Around line 379-424: Add tests in TestPluginImageResolverS3... to cover the
remaining existingVariantKey branches: one where fakeS3ImagePresigner reports
ObjectExists true for the requested w780 key so ResolveImageURLWithExpiry
returns the direct w780 presign and caches it with the existing-variant TTL, and
another where ObjectExists returns an error so the resolver still presigns the
requested key but does not cache the existence result. Use the existing
PluginImageResolver, SetS3Presigner, and fakeS3ImagePresigner helpers to assert
both the returned URL and the presigner call counts/keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2fa57f2c-1aa6-4b72-937a-861b0fb44ba5
📒 Files selected for processing (9)
internal/api/handlers/episode_image_variant_test.gointernal/api/handlers/items.gointernal/api/handlers/sections_test.gointernal/catalog/detail.gointernal/catalog/image_variants_test.gointernal/imagecache/imagecache.gointernal/imagecache/imagecache_test.gointernal/metadata/image_resolver.gointernal/metadata/image_resolver_test.go
db61955 to
e019603
Compare
0849a58 to
cdc62dd
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/catalog/detail.go (1)
3691-3701: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
postercase.The
postercase is identical in behavior to thedefaultcase. It can be removed to simplify the switch statement.♻️ Proposed refactor
- case "poster": - if size == "small" { - return "w300" - } - return "w500" default: if size == "small" { return "w300" } return "w500"🤖 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 `@internal/catalog/detail.go` around lines 3691 - 3701, Remove the redundant "poster" branch from the switch, leaving the existing default branch to handle it and preserve the current size-to-dimension behavior.
🤖 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 `@internal/catalog/detail.go`:
- Around line 3691-3701: Remove the redundant "poster" branch from the switch,
leaving the existing default branch to handle it and preserve the current
size-to-dimension behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a83db50a-bbaf-459d-871d-66026cc6ebb3
📒 Files selected for processing (5)
internal/api/handlers/episode_image_variant_test.gointernal/api/handlers/items.gointernal/api/handlers/sections_test.gointernal/catalog/detail.gointernal/catalog/image_variants_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/api/handlers/episode_image_variant_test.go
- internal/catalog/image_variants_test.go
- internal/api/handlers/sections_test.go
- internal/api/handlers/items.go
d189b08 to
dcbff12
Compare
dcbff12 to
2d46aa5
Compare
Add larger generated variants for TV-sized still and logo surfaces, while preserving existing fallbacks for cached images that have not yet been refreshed. Episode still responses use a still-specific variant path instead of the small card thumbnail path.
aadb26b to
cad0b41
Compare
|
Thanks @raiden202 — this PR correctly identified both the missing large TV variants and the need for an S3 existence-check fallback while older libraries catch up, and that design is carried forward with credit in #742, which supersedes this PR. #742 extends the same idea to posters (the most visible blur on 4K TVs), adds a client-facing |
2 participants
feat(images): add larger TV image variants
Problem
Large TV detail surfaces can upscale the existing small still/logo variants, which is especially noticeable on 4K displays. Episode stills also reuse a smaller card thumbnail path where a TV-sized still variant is more appropriate.
Approach
Add larger generated variants for TV-sized still and logo surfaces, use a still-specific episode image variant where needed, and preserve fallback behavior for cached images whose larger variants have not been generated yet. Fallback presigned URLs are cached only for the short missing-variant window, so a refresh-generated larger object is picked up promptly. Focused tests cover variant selection, generated variant ladders, and S3 resolver fallback from new keys to older cached variants.
Verification
go test ./internal/api/handlers ./internal/catalog ./internal/imagecache ./internal/metadata git diff --checkRisks & follow-up
Moderate risk. The change is additive and keeps existing small card surfaces on smaller variants, but operators should expect extra S3/storage and cache-warm bandwidth once images refresh: logos add
w1280alongside existing logo variants, and stills addw780alongside existing still variants. Existing cached media can continue to fall back when larger variants are unavailable, and missing larger variants are filled only when refresh/upload runs.AI-use disclosure
Implemented with a combination of Claude Code and Codex assistance, with human oversight.
Summary by CodeRabbit