Improve thumbnail handling(TS-2541) #1164
Conversation
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1164 +/- ##
==========================================
+ Coverage 92.23% 92.26% +0.02%
==========================================
Files 331 333 +2
Lines 10088 10170 +82
Branches 927 934 +7
==========================================
+ Hits 9305 9383 +78
- Misses 657 660 +3
- Partials 126 127 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Roffenlund
force-pushed
the
improve-thumbnail-handling
branch
from
ee7e2cb to
f08430c
Compare
Roffenlund
force-pushed
the
improve-thumbnail-handling
branch
from
6ae3291 to
04513dc
Compare
Roffenlund
force-pushed
the
improve-thumbnail-handling
branch
from
04513dc to
74cd293
Compare
Roffenlund
force-pushed
the
improve-thumbnail-handling
branch
2 times, most recently
from
819b517 to
bcabd80
Compare
Implement a service which either fetches and returns the url for an existing thumbnail of a given asset, or creates a new one if it does not exist. Refs. TS-2541
Implement a redirect view which redirects to the asset's thumbnail url if it exists. The view calls the service when trying to fetch the url. Add caching to the view so it caches in browser for 24h. Refs. TS-2541
Implement a custom templatetag to replace the existing easy_thumbnails templatetag. Refs. TS-2541
Use the cached_template templatetag for getting template urls in the templates instead of using the easy_thumbnails templatetag. Refs. TS-2541
Rewrite get() function to set the response to either a redirect or a 404 if the URL to the thumbnail is found. Update tests accordingly to these changes. Refs. TS-2541
Implement view for returning the thumbnail with a FileResponse instead of a redirect. Refs. TS-2541
Roffenlund
force-pushed
the
improve-thumbnail-handling
branch
from
bcabd80 to
e96aeda
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a cached thumbnail pipeline: new thumbnail service and dataclass, a template tag returning CDN thumbnail URLs, two views and URL routes to redirect/serve thumbnails, updates multiple templates to use the new tag, and adds unit tests for the service and views. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Browser
participant Template
participant Tag as cached_thumbnail.thumbnail_url
participant Router
participant RedirectV as ThumbnailRedirectView
participant ServeV as ThumbnailServeView
participant Service as get_or_create_thumbnail
participant Storage
Browser->>Template: Request page
Template->>Tag: thumbnail_url(image, W, H)
Tag-->>Browser: returns /thumbnail-serve/<path>?width=W&height=H
Browser->>Router: GET /thumbnail-serve/<path>?width=W&height=H
Router->>ServeV: dispatch GET
ServeV->>Service: get_or_create_thumbnail(path, W, H)
alt thumbnail exists or generated
Service-->>ServeV: Thumbnail(url, storage_path)
ServeV->>Storage: open(storage_path)
Storage-->>ServeV: file
ServeV-->>Browser: 200 FileResponse (image/*, Cache-Control: 24h)
else error or invalid params
Service-->>ServeV: None
ServeV-->>Browser: 404 (Cache-Control: 5m)
end
note over RedirectV,ServeV: Redirect view available at /thumbnail-redirect/... returns 302 -> thumbnail.url (24h) or 404 (5m)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
django/thunderstore/frontend/views.py (1)
103-105: Drop duplicate mimetype guess.Redundant call; keep the one in the try-block.
- mime_type, _ = mimetypes.guess_type(thumbnail_path) try: mime_type, _ = mimetypes.guess_type(thumbnail_path)
🧹 Nitpick comments (17)
django/thunderstore/repository/templates/repository/includes/dependencies.html (1)
13-13: Reduce CLS and improve loading behaviorAdd explicit dimensions and modern loading hints.
- <img class="align-self-center mr-3" src="{% thumbnail_url dependency.icon 64 64 %}" alt="{{ dependency }} icon"> + <img class="align-self-center mr-3" + src="{% thumbnail_url dependency.icon 64 64 %}" + width="64" height="64" loading="lazy" decoding="async" + alt="{{ dependency }} icon">django/thunderstore/community/templates/community/packagelisting_detail.html (1)
16-16: Prefer absolute og:image URLMany scrapers expect absolute URLs. After updating the tag to support absolute URLs, pass absolute=True here.
-<meta property="og:image" content="{% thumbnail_url object.package.icon 256 256 %}" /> +<meta property="og:image" content="{% thumbnail_url object.package.icon 256 256 absolute=True %}" />django/thunderstore/community/templates/community/packagelisting_versions.html (1)
13-13: Prefer absolute og:image URLAlign with share-preview best practices. Requires the tag change proposed earlier.
- <meta property="og:image" content="{% thumbnail_url object.package.icon 256 256 %}" /> + <meta property="og:image" content="{% thumbnail_url object.package.icon 256 256 absolute=True %}" />django/thunderstore/community/templates/community/packagelisting_changelog.html (1)
14-16: Make og:image absolute (some scrapers reject relative URLs)Confirm thumbnail_url returns an absolute URL; if not, prefix with scheme/host.
Apply if thumbnail_url is relative:
- <meta property="og:image" content="{% thumbnail_url object.package.icon 256 256 %}" /> + <meta property="og:image" content="{{ request.scheme }}://{{ request.get_host }}{% thumbnail_url object.package.icon 256 256 %}" />django/thunderstore/core/urls.py (1)
61-70: CDN/edge caching and tag coupling
- Ensure these endpoints are served behind CDN with caching enabled; 24h in-view headers are good, consider longer if invalidation is handled.
- Verify cached_thumbnail.thumbnail_url reverses cdn_thumb_serve (not cdn_thumb_redirect) to avoid extra hop.
django/thunderstore/community/templates/community/includes/package_header.html (1)
5-5: Add intrinsic dimensions to prevent CLSSpecify width/height (and async decode) for better layout stability.
- <img class="align-self-center mr-3" src="{% thumbnail_url object.package.icon 128 128 %}" alt="{{ object.package }} icon"> + <img class="align-self-center mr-3" src="{% thumbnail_url object.package.icon 128 128 %}" width="128" height="128" alt="{{ object.package }} icon" decoding="async">django/thunderstore/community/templates/community/packagelisting_list.html (1)
163-163: Lazy-load and set dimensions for grid thumbnailsReduces LCP/CLS and bandwidth on listing pages.
- <img class="w-100" src="{% thumbnail_url object.package.icon 256 256 %}" alt="{{ object.package }} icon"> + <img class="w-100" src="{% thumbnail_url object.package.icon 256 256 %}" width="256" height="256" alt="{{ object.package }} icon" loading="lazy" decoding="async">Also verify the new pipeline preserves the old “crop” behavior from easy_thumbnails for square icons.
django/thunderstore/repository/templates/repository/packageversion_detail.html (2)
15-17: Make og:image absoluteEnsure scrapers get a fully-qualified URL.
-<meta property="og:image" content="{% thumbnail_url object.icon 256 256 %}" /> +<meta property="og:image" content="{{ request.scheme }}://{{ request.get_host }}{% thumbnail_url object.icon 256 256 %}" />
66-66: Add intrinsic dimensions for the header iconPrevents layout shift; async decode is a small win.
- <img class="align-self-center mr-3" src="{% thumbnail_url object.icon 128 128 %}" alt="{{ object }} icon"> + <img class="align-self-center mr-3" src="{% thumbnail_url object.icon 128 128 %}" width="128" height="128" alt="{{ object }} icon" decoding="async">django/thunderstore/community/templates/community/community_list.html (1)
21-21: Add intrinsic size and lazy-load to reduce CLS and bandwidth.Include width/height and loading/decoding hints.
- <img class="w-100" src="{% thumbnail_url object.cover_image 360 480 %}" alt="{{ object.name }} icon"> + <img class="w-100" src="{% thumbnail_url object.cover_image 360 480 %}" width="360" height="480" loading="lazy" decoding="async" alt="{{ object.name }} icon">django/thunderstore/frontend/tests/services/test_thumbnail_service.py (3)
25-37: Rename variable for clarity (not a URL).The function returns a Thumbnail or None; rename the var.
- thumbnail_url = get_or_create_thumbnail(asset_path, width, height) + thumbnail = get_or_create_thumbnail(asset_path, width, height) - assert thumbnail_url is None + assert thumbnail is None
60-66: Fix mock name to match target and intent.Minor readability improvement.
- with patch(path) as mock_get_thumbnailer: + with patch(path) as mock_get_thumbnail: get_or_create_thumbnail(asset_path, width, height) - if thumbnail_exists: - mock_get_thumbnailer.assert_not_called() - else: - mock_get_thumbnailer.assert_called_once_with(options, generate=True) + if thumbnail_exists: + mock_get_thumbnail.assert_not_called() + else: + mock_get_thumbnail.assert_called_once_with(options, generate=True)
1-67: Ruff S101 in tests.If Ruff flags asserts in tests, consider excluding S101 for test paths in ruff config instead of refactoring.
Example ruff.toml:
[tool.ruff] ignore = [] [tool.ruff.per-file-ignores] "**/tests/**/*.py" = ["S101"]django/thunderstore/frontend/views.py (3)
3-3: Remove unused import.-from urllib.parse import urlparse
97-99: Cache negative responses for serve endpoint too.Aligns with redirect view and reduces repeated misses.
- if not asset_path or width <= 0 or height <= 0: - return HttpResponseNotFound("Invalid request parameters.") + if not asset_path or width <= 0 or height <= 0: + response = HttpResponseNotFound("Invalid request parameters.") + patch_cache_control(response, max_age=86400, public=True) + return response
110-113: Also cache 404s from storage misses and generic invalid cases.- except FileNotFoundError: - return HttpResponseNotFound("Thumbnail not found.") + except FileNotFoundError: + response = HttpResponseNotFound("Thumbnail not found.") + patch_cache_control(response, max_age=86400, public=True) + return response - return HttpResponseNotFound("Invalid request.") + response = HttpResponseNotFound("Invalid request.") + patch_cache_control(response, max_age=86400, public=True) + return responsedjango/thunderstore/frontend/tests/test_views.py (1)
200-214: Add negative-serve tests to lock in error caching.Extend coverage for invalid params and file-not-found on the serve endpoint.
@@ def test_thumbnail_serve_success(dummy_cover_image, client, community_site): @@ assert response.get("Content-Disposition").startswith("inline; filename=") + + +@pytest.mark.django_db +def test_thumbnail_serve_invalid_params_cached_404(dummy_cover_image, client, community_site): + community = community_site.community + community.cover_image = dummy_cover_image + community.save() + + url = reverse("cdn_thumb_serve", kwargs={"path": community.cover_image.name}) + response = client.get(url, {"width": 0, "height": 100}, HTTP_HOST=community_site.site.domain) + assert response.status_code == 404 + assert "Cache-Control" in response and "max-age=86400" in response["Cache-Control"] + + +@pytest.mark.django_db +def test_thumbnail_serve_storage_miss_cached_404(dummy_cover_image, client, community_site): + # Force get_or_create_thumbnail to return a bogus storage path + class DummyThumb: + storage_path = "nonexistent.jpg" + url = "" + + community = community_site.community + community.cover_image = dummy_cover_image + community.save() + + url = reverse("cdn_thumb_serve", kwargs={"path": community.cover_image.name}) + with patch("thunderstore.frontend.views.get_or_create_thumbnail", return_value=DummyThumb()), \ + patch("thunderstore.frontend.views.default_storage.open", side_effect=FileNotFoundError): + response = client.get(url, {"width": 100, "height": 100}, HTTP_HOST=community_site.site.domain) + assert response.status_code == 404 + assert "Cache-Control" in response and "max-age=86400" in response["Cache-Control"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
django/thunderstore/community/templates/community/community_list.html(2 hunks)django/thunderstore/community/templates/community/includes/package_header.html(1 hunks)django/thunderstore/community/templates/community/packagelisting_changelog.html(2 hunks)django/thunderstore/community/templates/community/packagelisting_detail.html(2 hunks)django/thunderstore/community/templates/community/packagelisting_list.html(2 hunks)django/thunderstore/community/templates/community/packagelisting_versions.html(2 hunks)django/thunderstore/core/urls.py(2 hunks)django/thunderstore/frontend/services/thumbnail.py(1 hunks)django/thunderstore/frontend/templatetags/cached_thumbnail.py(1 hunks)django/thunderstore/frontend/tests/services/test_thumbnail_service.py(1 hunks)django/thunderstore/frontend/tests/test_views.py(2 hunks)django/thunderstore/frontend/views.py(2 hunks)django/thunderstore/repository/templates/repository/includes/dependencies.html(2 hunks)django/thunderstore/repository/templates/repository/packageversion_detail.html(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
django/thunderstore/frontend/tests/services/test_thumbnail_service.py (2)
django/thunderstore/frontend/services/thumbnail.py (1)
get_or_create_thumbnail(14-35)django/conftest.py (1)
dummy_cover_image(484-489)
django/thunderstore/core/urls.py (1)
django/thunderstore/frontend/views.py (2)
ThumbnailRedirectView(60-84)ThumbnailServeView(87-113)
django/thunderstore/frontend/tests/test_views.py (2)
django/conftest.py (2)
dummy_cover_image(484-489)site(296-297)django/thunderstore/frontend/views.py (2)
get(77-84)get(88-113)
django/thunderstore/frontend/views.py (1)
django/thunderstore/frontend/services/thumbnail.py (1)
get_or_create_thumbnail(14-35)
🪛 Ruff (0.12.2)
django/thunderstore/frontend/services/thumbnail.py
34-34: Do not catch blind exception: Exception
(BLE001)
django/thunderstore/frontend/tests/services/test_thumbnail_service.py
19-19: Use of assert detected
(S101)
20-20: Use of assert detected
(S101)
21-21: Use of assert detected
(S101)
36-36: Use of assert detected
(S101)
django/thunderstore/frontend/tests/test_views.py
148-148: Use of assert detected
(S101)
149-149: Use of assert detected
(S101)
150-150: Use of assert detected
(S101)
151-151: Use of assert detected
(S101)
168-168: Use of assert detected
(S101)
169-169: Use of assert detected
(S101)
170-170: Use of assert detected
(S101)
195-195: Use of assert detected
(S101)
196-196: Use of assert detected
(S101)
197-197: Use of assert detected
(S101)
210-210: Use of assert detected
(S101)
211-211: Use of assert detected
(S101)
212-212: Use of assert detected
(S101)
213-213: Use of assert detected
(S101)
django/thunderstore/frontend/views.py
61-61: Unused method argument: args
(ARG002)
61-61: Unused method argument: kwargs
(ARG002)
77-77: Unused method argument: request
(ARG002)
88-88: Unused method argument: args
(ARG002)
88-88: Unused method argument: kwargs
(ARG002)
109-109: Consider moving this statement to an else block
(TRY300)
🪛 Flake8 (7.2.0)
django/thunderstore/frontend/views.py
[error] 3-3: 'urllib.parse.urlparse' imported but unused
(F401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build docker image
- GitHub Check: Analyze (javascript)
- GitHub Check: Build docker image
🔇 Additional comments (15)
django/thunderstore/frontend/templatetags/cached_thumbnail.py (1)
12-14: No action needed: URL pattern already uses<path:path>Thecdn_thumb_serveroute in django/thunderstore/core/urls.py is defined aspath("thumbnail-serve/<path:path>/", ThumbnailServeView.as_view(), name="cdn_thumb_serve")which correctly employs the
pathconverter.django/thunderstore/repository/templates/repository/includes/dependencies.html (1)
1-1: Template tag switch looks goodLoading cached_thumbnail is consistent with the new pipeline.
django/thunderstore/community/templates/community/packagelisting_detail.html (1)
2-2: Library swap to cached_thumbnail is consistentNo issues spotted with the load change.
django/thunderstore/community/templates/community/packagelisting_versions.html (1)
2-2: Library swap to cached_thumbnail is consistentNo issues spotted with the load change.
django/thunderstore/community/templates/community/packagelisting_changelog.html (1)
2-2: Template tag migration looks goodSwapping to cached_thumbnail is consistent with the new pipeline.
django/thunderstore/core/urls.py (1)
15-16: New imports align with added endpointsNo issues spotted.
django/thunderstore/community/templates/community/includes/package_header.html (1)
1-1: Template tag swap acknowledgedConsistent with the project-wide change.
django/thunderstore/community/templates/community/packagelisting_list.html (1)
2-2: Template tag migration LGTMMatches other templates.
django/thunderstore/repository/templates/repository/packageversion_detail.html (1)
2-2: Switch to cached_thumbnail confirmedNo concerns.
django/thunderstore/community/templates/community/community_list.html (1)
3-3: Switch to cached_thumbnail tag looks good.No functional concerns here.
django/thunderstore/frontend/tests/services/test_thumbnail_service.py (1)
10-22: Solid happy-path assertions.Covers format, quality/crop marker, and extension.
django/thunderstore/frontend/views.py (1)
77-85: Redirect logic with caching looks good.Matches tests (302 on success, 404 on failure) and sets 24h cache.
django/thunderstore/frontend/tests/test_views.py (3)
137-152: Redirect tests cover happy path and caching well.No changes needed.
155-171: Exception path is validated correctly.Matches 404 + caching behavior.
185-198: Invalid-param redirect coverage is good.Consistent 404 with cache headers.
| from dataclasses import dataclass | ||
| from typing import Optional | ||
|
|
||
| from django.conf import settings | ||
| from easy_thumbnails.files import get_thumbnailer | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid blind except and add logging
Catching Exception hides real failures. Narrow and log.
from dataclasses import dataclass
from typing import Optional
from django.conf import settings
from easy_thumbnails.files import get_thumbnailer
+from easy_thumbnails.exceptions import EasyThumbnailsError
+import logging
+
+logger = logging.getLogger(__name__)
@@
- except Exception:
- return None
+ except (OSError, EasyThumbnailsError):
+ logger.exception(
+ "Thumbnail generation failed",
+ extra={"asset_path": asset_path, "width": width, "height": height},
+ )
+ return NoneAlso applies to: 34-35
🤖 Prompt for AI Agents
In django/thunderstore/frontend/services/thumbnail.py around lines 1-6 and also
at 34-35, replace the blind "except Exception" handlers with targeted exception
handling and add logging: import Python's logging (logger =
logging.getLogger(__name__)) at the top, catch specific exceptions (e.g.,
OSError/IOError, ValueError, KeyError and the library-specific thumbnail/image
exceptions) instead of Exception, log the full exception context with
logger.exception(...) including which file/thumbnail operation failed, and then
either re-raise or return an explicit failure value (e.g., None) so failures are
not silently swallowed.
| thumbnail_options = { | ||
| "size": (width, height), | ||
| "crop": True, | ||
| "quality": settings.THUMBNAIL_QUALITY, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guardrails: validate size and prevent unbounded work
Add basic bounds and disable upscaling to avoid large/abusive requests exhausting CPU/memory.
thumbnail_options = {
- "size": (width, height),
+ "size": (width, height),
"crop": True,
"quality": settings.THUMBNAIL_QUALITY,
}Add near the top of the function:
def get_or_create_thumbnail(
asset_path: str, width: int, height: int
) -> Optional[Thumbnail]:
try:
+ if width <= 0 or height <= 0:
+ return None
+ max_w = getattr(settings, "THUMBNAIL_MAX_WIDTH", 1024)
+ max_h = getattr(settings, "THUMBNAIL_MAX_HEIGHT", 1024)
+ width = min(width, max_w)
+ height = min(height, max_h)And include:
- thumbnail_options = {
+ thumbnail_options = {
"size": (width, height),
"crop": True,
"quality": settings.THUMBNAIL_QUALITY,
+ "upscale": False,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| thumbnail_options = { | |
| "size": (width, height), | |
| "crop": True, | |
| "quality": settings.THUMBNAIL_QUALITY, | |
| } | |
| def get_or_create_thumbnail( | |
| asset_path: str, width: int, height: int | |
| ) -> Optional[Thumbnail]: | |
| try: | |
| # Guardrails: validate size and cap dimensions | |
| if width <= 0 or height <= 0: | |
| return None | |
| max_w = getattr(settings, "THUMBNAIL_MAX_WIDTH", 1024) | |
| max_h = getattr(settings, "THUMBNAIL_MAX_HEIGHT", 1024) | |
| width = min(width, max_w) | |
| height = min(height, max_h) | |
| thumbnail_options = { | |
| "size": (width, height), | |
| "crop": True, | |
| "quality": settings.THUMBNAIL_QUALITY, | |
| "upscale": False, | |
| } | |
| … |
🤖 Prompt for AI Agents
In django/thunderstore/frontend/services/thumbnail.py around lines 20 to 24, add
input validation near the top of the function to guard width/height: ensure they
are integers within acceptable bounds (e.g. >0 and <= a configured
MAX_THUMBNAIL_DIMENSION), normalize or clamp values if needed, and raise or
return an error for out-of-range values; then update thumbnail_options to
include disable upscaling (e.g. upscale=False) and enforce the validated/clamped
size so large or abusive requests cannot trigger unbounded CPU/memory work.
| thumbnail_name = thumbnailer.get_thumbnail_name(thumbnail_options) | ||
| if thumbnailer.source_storage.exists(thumbnail_name): | ||
| url = thumbnailer.source_storage.url(thumbnail_name) | ||
| storage_path = thumbnailer.name | ||
| return Thumbnail(storage_path=storage_path, url=url) | ||
| else: | ||
| thumbnail = thumbnailer.get_thumbnail(thumbnail_options, generate=True) | ||
| return Thumbnail(storage_path=thumbnail.name, url=thumbnail.url) |
There was a problem hiding this comment.
Bug: wrong storage used and incorrect storage_path when thumbnail exists
Use thumbnail storage (not source_storage) and return the thumbnail’s path, not the source image path.
- thumbnail_name = thumbnailer.get_thumbnail_name(thumbnail_options)
- if thumbnailer.source_storage.exists(thumbnail_name):
- url = thumbnailer.source_storage.url(thumbnail_name)
- storage_path = thumbnailer.name
- return Thumbnail(storage_path=storage_path, url=url)
+ thumbnail_name = thumbnailer.get_thumbnail_name(thumbnail_options)
+ storage = thumbnailer.thumbnail_storage
+ if storage.exists(thumbnail_name):
+ url = storage.url(thumbnail_name)
+ return Thumbnail(storage_path=thumbnail_name, url=url)
else:
thumbnail = thumbnailer.get_thumbnail(thumbnail_options, generate=True)
return Thumbnail(storage_path=thumbnail.name, url=thumbnail.url)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| thumbnail_name = thumbnailer.get_thumbnail_name(thumbnail_options) | |
| if thumbnailer.source_storage.exists(thumbnail_name): | |
| url = thumbnailer.source_storage.url(thumbnail_name) | |
| storage_path = thumbnailer.name | |
| return Thumbnail(storage_path=storage_path, url=url) | |
| else: | |
| thumbnail = thumbnailer.get_thumbnail(thumbnail_options, generate=True) | |
| return Thumbnail(storage_path=thumbnail.name, url=thumbnail.url) | |
| thumbnail_name = thumbnailer.get_thumbnail_name(thumbnail_options) | |
| storage = thumbnailer.thumbnail_storage | |
| if storage.exists(thumbnail_name): | |
| url = storage.url(thumbnail_name) | |
| return Thumbnail(storage_path=thumbnail_name, url=url) | |
| else: | |
| thumbnail = thumbnailer.get_thumbnail(thumbnail_options, generate=True) | |
| return Thumbnail(storage_path=thumbnail.name, url=thumbnail.url) |
🤖 Prompt for AI Agents
In django/thunderstore/frontend/services/thumbnail.py around lines 26 to 33, the
code checks and returns the existing thumbnail using thumbnailer.source_storage
and sets storage_path to thumbnailer.name (the source image) which is wrong;
change the existence and url checks to use thumbnailer.storage (the thumbnail
storage) and set storage_path to the thumbnail path (thumbnail_name) when the
file exists, so use thumbnailer.storage.exists(thumbnail_name),
thumbnailer.storage.url(thumbnail_name) and set storage_path=thumbnail_name;
keep the existing branch that generates and returns thumbnail.name/url for the
non-existing case.
| @register.simple_tag | ||
| def thumbnail_url(image_field, width: int, height: int) -> str: | ||
| if not image_field: | ||
| return "" | ||
|
|
||
| url = ( | ||
| reverse("cdn_thumb_serve", kwargs={"path": image_field.name}) | ||
| + f"?width={width}&height={height}" | ||
| ) | ||
|
|
||
| return url |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enable absolute URLs for OG/meta use and build querystring safely
OG scrapers prefer absolute URLs. Also, use urlencode and allow an optional absolute flag without breaking existing callers.
-from django import template
+from django import template
+from urllib.parse import urlencode
@@
-@register.simple_tag
-def thumbnail_url(image_field, width: int, height: int) -> str:
+@register.simple_tag(takes_context=True)
+def thumbnail_url(context, image_field, width: int, height: int, *, absolute: bool = False) -> str:
if not image_field:
return ""
- url = (
- reverse("cdn_thumb_serve", kwargs={"path": image_field.name})
- + f"?width={width}&height={height}"
- )
+ base = reverse("cdn_thumb_serve", kwargs={"path": image_field.name})
+ qs = urlencode({"width": int(width), "height": int(height)})
+ url = f"{base}?{qs}"
+ if absolute:
+ request = context.get("request")
+ if request:
+ url = request.build_absolute_uri(url)
return url📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @register.simple_tag | |
| def thumbnail_url(image_field, width: int, height: int) -> str: | |
| if not image_field: | |
| return "" | |
| url = ( | |
| reverse("cdn_thumb_serve", kwargs={"path": image_field.name}) | |
| + f"?width={width}&height={height}" | |
| ) | |
| return url | |
| from django import template | |
| from urllib.parse import urlencode | |
| register = template.Library() | |
| @register.simple_tag(takes_context=True) | |
| def thumbnail_url(context, image_field, width: int, height: int, *, absolute: bool = False) -> str: | |
| if not image_field: | |
| return "" | |
| base = reverse("cdn_thumb_serve", kwargs={"path": image_field.name}) | |
| qs = urlencode({"width": int(width), "height": int(height)}) | |
| url = f"{base}?{qs}" | |
| if absolute: | |
| request = context.get("request") | |
| if request: | |
| url = request.build_absolute_uri(url) | |
| return url |
🤖 Prompt for AI Agents
In django/thunderstore/frontend/templatetags/cached_thumbnail.py around lines
7-17, change the tag to accept an optional absolute flag without breaking
existing callers and build the querystring with urlencode: make the tag use
@register.simple_tag(takes_context=True) and signature def
thumbnail_url(context, image_field, width: int, height: int, absolute: bool =
False) -> str, import urlencode from urllib.parse, construct params = {"width":
width, "height": height}, query = urlencode(params), build path =
reverse("cdn_thumb_serve", kwargs={"path": image_field.name}) and url =
f"{path}?{query}", and if absolute and context.get("request") is present return
context["request"].build_absolute_uri(url) else return the relative url; default
absolute False preserves existing behavior.
Improve how patch_cache_control is used in the views. Cache failures for 5 minutes instead of 24h. Only cache valid responses 24h. Add tests for ThumbnailServeView. Refs. TS-2541
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
django/thunderstore/frontend/views.py (1)
77-89: FYI: 404 vs 410 inconsistency from prior review is resolvedEarlier feedback suggested returning 410 for failed thumbnail generation. Current code returns 404 and tests assert 404, so the discrepancy is gone. No change needed.
🧹 Nitpick comments (7)
django/thunderstore/frontend/views.py (6)
3-3: Remove unused import
urlparseisn’t used. Drop it to satisfy Flake8 F401.-from urllib.parse import urlparse
61-76: Use kwargs (not self.kwargs) to avoid unused-args warning; keep signature cleanLeverage
kwargspassed intoget_redirect_urlto quiet Ruff ARG002 and reduce reliance on view state.-class ThumbnailRedirectView(RedirectView): - def get_redirect_url(self, *args, **kwargs) -> str: - asset_path = self.kwargs.get("path") +class ThumbnailRedirectView(RedirectView): + def get_redirect_url(self, *args, **kwargs) -> str: # noqa: ARG002 + asset_path = kwargs.get("path") url = ""
77-89: Tighten signature and fix misleading comment
- Prefix
requestwith_to silence Ruff ARG002 (unused).- Remove the “# 5 minutes” comment at the cache patch site since
max_agemay be 24h.- def get(self, request, *args, **kwargs): + def get(self, _request, *args, **kwargs): # noqa: ARG002 @@ - patch_cache_control(response, max_age=max_age, public=True) # 5 minutes + patch_cache_control(response, max_age=max_age, public=True)
92-121: Harden file serving: explicit Content-Disposition and MIME fallback; quiet unused args
- Add a default MIME type when
mimetypescan’t guess.- Set an explicit inline Content-Disposition for consistency across Django/proxies.
- Prefix unused var names to satisfy Ruff.
-class ThumbnailServeView(View): - def get(self, request, *args, **kwargs): +class ThumbnailServeView(View): + def get(self, request, *_args, **_kwargs): # noqa: ARG002 asset_path = self.kwargs.get("path") @@ - if thumbnail_path: + if thumbnail_path: try: - mime_type, _ = mimetypes.guess_type(thumbnail_path) - file = default_storage.open(thumbnail_path, "rb") - response = FileResponse(file, content_type=mime_type) + mime_type, _ = mimetypes.guess_type(thumbnail_path) + if not mime_type: + mime_type = "image/jpeg" + file = default_storage.open(thumbnail_path, "rb") + response = FileResponse(file, content_type=mime_type) + response["Content-Disposition"] = f'inline; filename="{thumbnail_path.rsplit("/", 1)[-1]}"' max_age = 86400 # 24h except FileNotFoundError: response = HttpResponseNotFound("Thumbnail not found.")
66-70: DRY up duplicate dimension parsingBoth views parse
width/heightidentically. Factor into a small helper to reduce error surface.# Suggested helper (place near other view utilities) def _parse_dimensions(request) -> tuple[int, int]: try: w = int(request.GET.get("width", 0)) h = int(request.GET.get("height", 0)) except (ValueError, TypeError): return 0, 0 return w, hThen use:
- Redirect:
width, height = _parse_dimensions(self.request)- Serve:
width, height = _parse_dimensions(request)Also applies to: 96-100
71-75: Guardrails: bound requested dimensions to prevent abuseConsider clamping
width/heightto a sane upper limit (e.g., 2048) to avoid expensive thumbnail generation on arbitrary inputs.Also applies to: 103-119
django/thunderstore/frontend/tests/test_views.py (1)
229-229: Remove duplicate db markerThere are two consecutive
@pytest.mark.django_dbdecorators beforetest_thumbnail_serve_exception. One is enough.-@pytest.mark.django_db def test_thumbnail_serve_exception(params, dummy_cover_image, client, community_site):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
django/thunderstore/frontend/tests/test_views.py(2 hunks)django/thunderstore/frontend/views.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
django/thunderstore/frontend/views.py (1)
django/thunderstore/frontend/services/thumbnail.py (1)
get_or_create_thumbnail(14-35)
django/thunderstore/frontend/tests/test_views.py (2)
django/conftest.py (2)
dummy_cover_image(484-489)site(296-297)django/thunderstore/frontend/views.py (2)
get(77-88)get(92-121)
🪛 Flake8 (7.2.0)
django/thunderstore/frontend/views.py
[error] 3-3: 'urllib.parse.urlparse' imported but unused
(F401)
🪛 Ruff (0.12.2)
django/thunderstore/frontend/views.py
61-61: Unused method argument: args
(ARG002)
61-61: Unused method argument: kwargs
(ARG002)
77-77: Unused method argument: request
(ARG002)
92-92: Unused method argument: args
(ARG002)
92-92: Unused method argument: kwargs
(ARG002)
django/thunderstore/frontend/tests/test_views.py
148-148: Use of assert detected
(S101)
149-149: Use of assert detected
(S101)
150-150: Use of assert detected
(S101)
151-151: Use of assert detected
(S101)
168-168: Use of assert detected
(S101)
169-169: Use of assert detected
(S101)
170-170: Use of assert detected
(S101)
195-195: Use of assert detected
(S101)
196-196: Use of assert detected
(S101)
197-197: Use of assert detected
(S101)
210-210: Use of assert detected
(S101)
211-211: Use of assert detected
(S101)
212-212: Use of assert detected
(S101)
213-213: Use of assert detected
(S101)
238-238: Use of assert detected
(S101)
239-239: Use of assert detected
(S101)
252-252: Use of assert detected
(S101)
253-253: Use of assert detected
(S101)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run pre-commit
- GitHub Check: Build docker image
- GitHub Check: Analyze (javascript)
- GitHub Check: Build docker image
🔇 Additional comments (2)
django/thunderstore/frontend/tests/test_views.py (2)
163-171: Patch target looks correctPatching
thunderstore.frontend.views.get_or_create_thumbnailaligns with how the view imports the function. Good.
200-214: Happy path coverage looks solidAsserts validate status, content type, cache headers, and inline disposition. Nice.
1 participant
Summary by CodeRabbit