Disable thumbnailing in private/draft projects for the time being#1340
Merged
Conversation
Draft (non-public) projects stopped displaying capture images after the server-side thumbnail layer (PR #1306) shipped. The thumbnail endpoint is authentication-gated, but the frontend loads thumbnails via an anonymous <img> tag, which cannot send an Authorization header. For a draft project the anonymous request is denied (401) and the image renders broken; public projects were unaffected because anonymous read passes their permission check. Interim fix: SourceImageThumbnailSerializer.get_thumbnails returns None for captures in draft projects, so the frontend falls back to the capture's presigned source URL (the pre-thumbnail-layer behavior, which authenticates via its own signature and stays private). Public projects keep server thumbnails. The draft lookup is memoized per project on the serializer instance to avoid an N+1, since the captures queryset does not select_related the SourceImage.project FK. A signed-token or per-project-storage thumbnail URL is the longer-term fix so draft projects also benefit from thumbnails; this change just unbreaks them without exposing any imagery in public default storage. Co-Authored-By: Claude <noreply@anthropic.com>
✅ Deploy Preview for antenna-ssec canceled.
|
✅ Deploy Preview for antenna-preview canceled.
|
Contributor
📝 WalkthroughWalkthroughAdds ChangesThumbnail gating for draft/private projects
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Restores capture image rendering for draft (non-public) projects by preventing the API from returning auth-gated thumbnail endpoint URLs for those projects, allowing the frontend to fall back to the capture’s presigned source URL (as before the thumbnail layer).
Changes:
- Omit
thumbnailsURLs in capture serialization when the capture’s project isdraft=True(public projects unchanged). - Add per-serializer memoization for the draft-project check to avoid per-capture DB queries.
- Add regression tests and a planning write-up documenting root cause and longer-term options.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| docs/claude/planning/2026-06-17-thumbnail-auth-private-projects.md | Adds decision record / root-cause analysis for the thumbnail auth issue on restricted projects. |
| ami/main/tests.py | Adds regression tests for draft vs public thumbnail serialization and a query-count guard. |
| ami/main/api/serializers.py | Omits thumbnail endpoint URLs for draft projects with a memoized draft lookup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mihow
changed the title
- Make the N+1 guard reliable: disable query caching and clear the cache so the captured query count reflects real DB hits, assert the draft lookup runs exactly once across a multi-capture response, and match the existence probe case-insensitively without quoted identifiers (not tied to one DB backend). - Call super().setUp() before authenticating the test client. - Reword the serializer comment to state what the code enforces (omitting thumbnail URLs so the UI falls back to the presigned source URL); note that endpoint-level enforcement and per-project thumbnail storage are follow-ups. Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai full review - safe to merge? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the PR diff
gh pr view 1340 --repo RolnickLab/antenna --json files,additions,deletions,changedFiles
gh pr diff 1340 --repo RolnickLab/antennaLength of output: 18633 🏁 Script executed: #!/bin/bash
# Look at the SourceImageThumbnailSerializer class and surrounding context in serializers.py
rg -n "SourceImageThumbnailSerializer|from ami|import Project|project_id|draft" ami/main/api/serializers.py | head -80
echo "=== Class definition context around get_thumbnails ==="
rg -n -B 40 "def get_thumbnails" ami/main/api/serializers.py | head -80
echo "=== Imports at top of serializers.py ==="
head -50 ami/main/api/serializers.py
echo "=== Project model draft field ==="
rg -n "draft" ami/main/models.py | grep -i "project\|draft" | head -20
echo "=== SourceImage.project_id field ==="
rg -n "project" ami/main/models.py | grep "SourceImage\|project_id\|ForeignKey.*Project" | head -20
echo "=== check test helpers used (setup_test_project, create_captures_from_files) ==="
rg -n "def setup_test_project|def create_captures_from_files" ami/main/tests.py | head -10
echo "=== existing TestSourceImageThumbnail context ==="
rg -n -A5 -B5 "class TestSourceImageThumbnail" ami/main/tests.py | head -30
echo "=== override_settings import in tests.py ==="
rg -n "override_settings|from django" ami/main/tests.py | head -20Length of output: 9530 🏁 Script executed: #!/bin/bash
echo "=== Find setup_test_project and create_captures_from_files definitions ==="
rg -rn "def setup_test_project|def create_captures_from_files" ami/
echo "=== APITestCase import in tests.py ==="
rg -n "APITestCase" ami/main/tests.py | head -10
echo "=== Check SourceImage.project FK on_delete and null ==="
rg -n -A3 "project = models.ForeignKey" ami/main/models.py | grep -A3 "captures\|SourceImage" | head -20
echo "=== Check subclasses that inherit SourceImageThumbnailSerializer ==="
rg -n "SourceImageThumbnailSerializer" ami/main/api/serializers.py
echo "=== Check if _draft_project_cache initialized anywhere ==="
rg -n "_draft_project_cache" ami/
echo "=== Check the captures list endpoint to understand project_id query param ==="
rg -n "project_id" ami/main/api/views.py | grep -i "filter\|param\|capture" | head -20
echo "=== Check existing thumbnail test class ==="
rg -n -A 5 "class TestSourceImageThumbnail" ami/main/tests.py | head -30
echo "=== Look at create_captures_from_files call in existing tests ==="
rg -n "create_captures_from_files\|setup_test_project" ami/main/tests.py | head -20Length of output: 4101 🏁 Script executed: #!/bin/bash
echo "=== Look at imports section of ami/main/tests.py ==="
head -30 ami/main/tests.py
echo "=== Search for setup_test_project anywhere in tests.py ==="
grep -n "setup_test_project\|create_captures_from_files" ami/main/tests.py
echo "=== What's in ami/tests/fixtures/main.py ==="
cat ami/tests/fixtures/main.py | head -80
echo "=== Check if there is a conftest or fixture import that brings these in ==="
rg -rn "create_captures_from_files\|setup_test_project" ami/main/tests.py ami/tests/
echo "=== Look at the full new test class as it appears in the file ==="
grep -n -A 80 "class TestThumbnailDraftProjectVisibility" ami/main/tests.pyLength of output: 10615
Review Summary: ✅ Safe to mergeSerializer fix (
|
| Check | Result |
|---|---|
Project already imported at module level |
✅ |
obj.project_id is the integer FK column (no extra query) |
✅ |
project_id is None guard handles SET_NULL FK correctly |
✅ |
Memoization via getattr/dict is correct and thread-safe for this lifecycle |
✅ |
All three subclasses inherit the fix: ExampleSourceImageNestedSerializer, SourceImageListSerializer, EventCaptureNestedSerializer |
✅ |
| Public projects unaffected | ✅ |
Tests (ami/main/tests.py)
| Check | Result |
|---|---|
setup_test_project / create_captures_from_files imported at lines 50 / 46 |
✅ |
APITestCase imported from rest_framework.test |
✅ |
| All three test scenarios covered (public→thumbnails, draft→null, memoization) | ✅ |
@override_settings(CACHALOT_ENABLED=False) correctly prevents cachalot from masking a real N+1 |
✅ |
SQL pattern match uses .lower() — reasonably backend-agnostic |
✅ |
Two minor observations (non-blocking)
-
_draft_project_cachenot initialized in__init__: The lazygetattrpattern works correctly, but initializingself._draft_project_cache: dict[int, bool] = {}inSourceImageThumbnailSerializer.__init__would make the intent more visible to future readers. -
Weak fallback URL assertion:
self.assertTrue(rec["url"])intest_draft_project_omits_thumbnail_urlsonly checks for truthiness. A stronger assertion likeself.assertIn("http", rec["url"])would be more explicit — but this is a cosmetic point.
Acknowledged known gap
The code comment correctly flags that an authenticated API caller can still trigger thumbnail generation for draft projects into public default storage. This is a deliberate, well-documented deferral — not a regression introduced here — and the planning doc maps out the right long-term fix directions. No action needed in this PR.
The fix is deliberate, minimal, well-commented, and comes with solid regression tests that fill the gap (#1306 only tested public projects). Good to go. 🐇
Rework of the interim draft-project thumbnail fix. Replaces the per-row
Project.objects.filter(...).exists() lookup inside the serializer (and the
per-instance memoization cache it required) with a Project.thumbnails_enabled
property and select_related("project") on the captures list queryset.
- Project.thumbnails_enabled (= not draft) names the policy as domain logic on
the model, and is a proxy for "anonymous can retrieve captures"; revisit if
visibility ever decouples from the draft flag.
- SourceImageThumbnailSerializer.get_thumbnails reads obj.project.thumbnails_enabled
instead of issuing its own query, so the FK comes from the main JOIN and there
is no N+1 and no memoization to maintain. ~17 lines of comment collapse to a
short note pointing at the tracking issue.
- The N+1 regression test now asserts no standalone per-row project fetch occurs
(the select_related guarantee), rather than that a custom cache ran once.
- Inline comment and planning doc now reference the dedicated tracking issue
(#1341) for the real fix (self-authenticating / per-project-storage thumbnail
URLs) instead of inlining the rationale.
Co-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/claude/planning/2026-06-17-thumbnail-auth-private-projects.md (1)
121-132: 💤 Low valueEscape the
#1331references.Those inline issue references can be parsed as headings in Markdown, which breaks the intended prose rendering in this doc. Use backticks or escape the
#so the text stays inline.🤖 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 `@docs/claude/planning/2026-06-17-thumbnail-auth-private-projects.md` around lines 121 - 132, The inline issue references `#1331` in the markdown document are being parsed as Markdown headings, which breaks the prose rendering. To fix this, escape or wrap all instances of the `#1331` references in the document by either using backticks around them (like `#1331`) or by escaping the hash character with a backslash (like \`#1331`). This will preserve the inline formatting while preventing Markdown parsers from treating them as heading syntax.Source: Linters/SAST tools
🤖 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 `@docs/claude/planning/2026-06-17-thumbnail-auth-private-projects.md`:
- Around line 121-132: The inline issue references `#1331` in the markdown
document are being parsed as Markdown headings, which breaks the prose
rendering. To fix this, escape or wrap all instances of the `#1331` references
in the document by either using backticks around them (like `#1331`) or by
escaping the hash character with a backslash (like \`#1331`). This will preserve
the inline formatting while preventing Markdown parsers from treating them as
heading syntax.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 906e36b1-92ea-498e-84e1-d08b8ae3d9e3
📒 Files selected for processing (5)
ami/main/api/serializers.pyami/main/api/views.pyami/main/models.pyami/main/tests.pydocs/claude/planning/2026-06-17-thumbnail-auth-private-projects.md
The rationale (anonymous <img> can't authenticate the gated endpoint, interim fix, #1341 tracking) already lives in Project.thumbnails_enabled. Leave only a one-line pointer at the call site. Co-Authored-By: Claude <noreply@anthropic.com>
mihow
force-pushed
the
fix/thumbnail-draft-projects
branch
from
efe7350 to
8baab3c
Compare
The policy is self-documenting via Project.thumbnails_enabled, so the call-site comment is removed. Bucket type / presigned URLs are independent of project visibility: a private (draft) project may live on a public bucket and a public project on a private one. The fallback is just the capture's source URL (capture.url), which is presigned only when its bucket is private — so the tests no longer call it "presigned". Co-Authored-By: Claude <noreply@anthropic.com>
mihow
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Images in draft (non-public) projects have been rendering broken since the server-side thumbnail layer (#1306) shipped. This restores them.
The thumbnail endpoint is authentication-gated, but the frontend loads thumbnails through an anonymous
<img>tag, which cannot send an Authorization header. For a draft project the anonymous request is denied (401), so every capture image appears broken. Public projects were unaffected, because anonymous read passes their permission check.The fix is deliberately small and safe: for draft projects the API no longer hands back thumbnail-endpoint URLs, so the frontend falls back to the capture's source URL (
capture.url) — exactly how images were served before the thumbnail layer, bypassing the auth-gated endpoint. Public projects keep server-generated thumbnails and their performance benefit.(Note: project visibility is independent of storage. Whether the source URL is presigned depends on the capture's bucket, not on whether the project is draft — a draft project may live on a public bucket and a public project on a private one. The determinant here is the
draftflag / anonymous-retrieve permission, not bucket type.)This is an interim fix. The longer-term plan (self-authenticating signed-token thumbnail URLs, or writing thumbnails to each project's own object store) lets draft/private projects benefit from thumbnails too; it is tracked in #1341. We chose not to simply open the endpoint to anonymous access, because generated thumbnails are written to public default storage where they would be world-readable and enumerable by capture id — a privacy regression for restricted imagery.
List of Changes
Project.thumbnails_enabledproperty (= not draft) names the policy on the model;SourceImageThumbnailSerializer.get_thumbnailsreturnsNonewhen it is false, and the frontend already falls back to the source URL when no thumbnails are present (ui/.../capture.ts).select_related("project"), so the serializer readsobj.project.thumbnails_enabledstraight off the main JOIN — one query for the whole page, no per-instance memoization.TestThumbnailDraftProjectVisibility: public project returns thumbnail URLs; draft project omits them (and still returns the sourceurlfor fallback); and a query-capture test proves the list path does no standalone per-row project fetch (theselect_relatedguarantee). The existing thumbnail tests only covered a public project, which is why this regressed silently.Detailed Description
Root cause, empirical confirmation, the
<img>-cannot-authenticate constraint, and the longer-term options are tracked in #1341. Context also posted on #1306.Follow-up to #1306; interacts with #1331 (direct thumbnail URLs), which does not on its own fix draft projects. The durable fix is tracked in #1341.