Fix misplaced bounding boxes on portrait captures and add large image thumbnail size#1374
Open
mihow wants to merge 3 commits into
Open
Fix misplaced bounding boxes on portrait captures and add large image thumbnail size#1374mihow wants to merge 3 commits into
mihow wants to merge 3 commits into
Conversation
Portrait photos are typically stored as landscape pixels plus an EXIF Orientation tag. Browsers force-apply that rotation to cross-origin images, while detections, image dimensions, and crops all live in the raw (unrotated) pixel space - so the session detail view drew bounding boxes 90 degrees out of place on portrait captures. This regressed in PR #1339, which switched the view from the medium thumbnail to the original image URL for zoom quality. Thumbnails are re-encoded without EXIF metadata, so they always render in the same pixel space as the boxes. Add a "large" (2560px) thumbnail size for zoom quality and point the session detail view at it, falling back to the original URL when thumbnails are unavailable. Co-Authored-By: Claude <noreply@anthropic.com>
✅ Deploy Preview for antenna-ssec ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for antenna-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📝 WalkthroughWalkthroughAdds a 2560-pixel ChangesLarge thumbnail support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SessionDetailsPage
participant CaptureModel
participant CaptureComponent
SessionDetailsPage->>CaptureModel: Read thumbnailLarge
CaptureModel-->>SessionDetailsPage: Return large thumbnail URL or capture URL
SessionDetailsPage->>CaptureComponent: Pass image source and capture dimensions
CaptureComponent->>CaptureComponent: Scale boxes from capture dimensions
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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)
ami/main/tests.py (1)
297-297: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
unittestassertions for consistency.Consider using
self.assertIsNotNone()instead of a bareassertto maintain consistency with the rest of the assertions in this test case.💡 Proposed change
- assert self.deployment.data_source is not None + self.assertIsNotNone(self.deployment.data_source)🤖 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 `@ami/main/tests.py` at line 297, Replace the bare assertion on self.deployment.data_source with the test case’s unittest-style self.assertIsNotNone() assertion, preserving the same validation and message if one is already used nearby.
🤖 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 `@ami/main/tests.py`:
- Line 297: Replace the bare assertion on self.deployment.data_source with the
test case’s unittest-style self.assertIsNotNone() assertion, preserving the same
validation and message if one is already used nearby.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81bd9779-c2fb-46d3-8ecd-44330829d772
📒 Files selected for processing (4)
ami/main/tests.pyconfig/settings/base.pyui/src/data-services/models/capture.tsui/src/pages/session-details/session-details.tsx
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes misaligned detection bounding boxes on portrait captures in the session detail view by switching the UI to render a generated EXIF-stripped thumbnail (new “large” size) instead of the original image, ensuring the displayed pixels stay in the same raw coordinate space as stored detection boxes.
Changes:
- Session detail view now uses a “large” thumbnail URL as the capture image source (avoids EXIF orientation rendering differences in browsers).
- Added a new
largethumbnail size (2560px) to backend thumbnail settings. - Added backend tests to pin “large” thumbnail generation behavior and ensure EXIF orientation is stripped without rotating pixels.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
ui/src/pages/session-details/session-details.tsx |
Swaps the rendered capture source to the EXIF-stripped large thumbnail for correct box overlay alignment. |
ui/src/data-services/models/capture.ts |
Adds a thumbnailLarge accessor to prefer the large thumbnail URL (with fallback to original URL). |
config/settings/base.py |
Registers the new large (2560px) thumbnail size in THUMBNAILS["SIZES"]. |
ami/main/tests.py |
Adds coverage for label=large generation and for stripping EXIF orientation while preserving raw pixel dimensions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Box coordinates are stored in the original image's pixel space, and the session detail view now renders a downscaled thumbnail, so the previous fallback to the rendered image's natural size would draw boxes at the wrong scale for captures with no stored width/height. Skip drawing instead. Also document in the thumbnail generator that EXIF must not propagate to the output, since box alignment depends on it. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
mihow
force-pushed
the
fix/session-detail-thumbnail-orientation
branch
from
July 20, 2026 23:44
672de61 to
1a15510
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ui/src/pages/session-details/capture/capture.tsx (1)
89-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving the dimension check outside the
reduceloop.Since the absence of
widthorheightapplies to the entire capture, you can short-circuit before iterating over thedetectionsarray. This avoids evaluating the dimension check for every single detection.♻️ Proposed refactor
- const boxStyles = useMemo( - () => - detections.reduce((result: { [key: string]: BoxStyle }, detection) => { - const [boxLeft, boxTop, boxRight, boxBottom] = detection.bbox - const boxWidth = boxRight - boxLeft - const boxHeight = boxBottom - boxTop - - // Box coordinates are in the original image's pixel space, so they can - // only be scaled against the capture's stored dimensions. The rendered - // image may be a downscaled thumbnail, so its natural size is not a - // valid substitute — without stored dimensions, skip drawing boxes - // rather than drawing them at the wrong scale. - if (!width || !height) { - return result - } - - result[detection.id] = { - width: `${(boxWidth / width) * 100}%`, - height: `${(boxHeight / height) * 100}%`, - top: `${(boxTop / height) * 100}%`, - left: `${(boxLeft / width) * 100}%`, - } - - return result - }, {}), - [width, height, detections] - ) + const boxStyles = useMemo(() => { + // Box coordinates are in the original image's pixel space, so they can + // only be scaled against the capture's stored dimensions. The rendered + // image may be a downscaled thumbnail, so its natural size is not a + // valid substitute — without stored dimensions, skip drawing boxes + // rather than drawing them at the wrong scale. + if (!width || !height) { + return {} + } + + return detections.reduce((result: { [key: string]: BoxStyle }, detection) => { + const [boxLeft, boxTop, boxRight, boxBottom] = detection.bbox + const boxWidth = boxRight - boxLeft + const boxHeight = boxBottom - boxTop + + result[detection.id] = { + width: `${(boxWidth / width) * 100}%`, + height: `${(boxHeight / height) * 100}%`, + top: `${(boxTop / height) * 100}%`, + left: `${(boxLeft / width) * 100}%`, + } + + return result + }, {}) + }, [width, height, detections])🤖 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 `@ui/src/pages/session-details/capture/capture.tsx` around lines 89 - 115, Move the width/height guard outside the detections.reduce call in the boxStyles useMemo callback, returning an empty style map immediately when either dimension is unavailable; otherwise run the existing reduction and preserve the current box scaling 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 `@ui/src/pages/session-details/capture/capture.tsx`:
- Around line 89-115: Move the width/height guard outside the detections.reduce
call in the boxStyles useMemo callback, returning an empty style map immediately
when either dimension is unavailable; otherwise run the existing reduction and
preserve the current box scaling behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58e3e922-e89f-4904-8819-4fe6a7740c2e
📒 Files selected for processing (2)
ami/main/models.pyui/src/pages/session-details/capture/capture.tsx
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
Captures taken in portrait orientation (for example from a phone-based station) showed their detection bounding boxes in the wrong place in the session detail view — shifted and rotated 90° relative to the insects they belong to. This PR makes the session detail view render captures from a generated thumbnail instead of the original file. What users get today: correctly placed boxes on portrait captures, and faster capture browsing than scrubbing through full-size originals. A new 2560px "large" thumbnail size keeps zooming reasonably sharp.
List of Changes
Detailed Description
Photos shot in portrait are typically stored as landscape pixels plus an EXIF Orientation tag. Everything in our pipeline — detection coordinates, stored image dimensions, crops — lives in that raw (unrotated) pixel space and is mutually consistent. Browsers, however, force-apply the EXIF rotation when displaying the original file, and for cross-origin images they ignore the
image-orientation: noneCSS override unless the storage bucket serves CORS headers (verified empirically). So the detail view displayed a rotated image while the box overlay math worked in raw space.This regressed in #1339, which switched the view's image source from the medium thumbnail to the original URL to improve zoom quality. Generated thumbnails are re-encoded without EXIF metadata, so they always render in the same pixel space as the boxes — a new test makes that invariant explicit instead of a side effect, and a comment in the generator guards against a future "preserve metadata" change silently breaking every overlay.
All thumbnail plumbing (viewset label validation,
thumbnail_urls(), serializers) iteratessettings.THUMBNAILS["SIZES"]dynamically, so adding the "large" size required no code changes beyond the setting itself. PIL'sthumbnail()never upscales: sources narrower than 2560px get an original-resolution re-encode, which is still EXIF-stripped and therefore still correct.What happens to captures that already have thumbnails
Adding a size is purely additive. There is nothing to invalidate, backfill, or migrate before or after deploying this.
Thumbnails are stored as one row per capture and label (
SourceImageThumbnail, with a unique constraint on that pair), so each size is independent of the others. This change only adds alargeentry; thesmall(240px) andmedium(1024px) specs are untouched.thumbnail_is_valid()compares a stored row's width against the configured spec width, so existing rows remain valid and are never re-encoded.For an existing capture there is simply no
largerow yet, which reads as invalid.thumbnail_urls()therefore emits the route URL (/captures/thumbnails/<pk>/?label=large) for that label rather than a direct storage URL, and the first request generates the derivative lazily: fetch the original, encode a 2560px JPEG, store it, write the row, then redirect to storage. Every later request for that capture is warm and served straight from storage, with the backend out of the loop.The same mechanism covers invalidation in general. Because the stored width is the requested spec width, changing an existing size in settings (for example
mediumfrom 1024 to 1200) makes every row for that label read as invalid and regenerate on next request; a modified source image is handled the same way throughlast_modified. There is no cache-busting step to run and no management command to schedule — thumbnail generation only ever happens lazily, through the thumbnail viewset.Known caveats
largewhen someone actually zooms in, so the 2560px derivative gets generated for captures people inspect rather than every capture they scroll past. The two are intended to merge and deploy together for that reason. Pre-generating thumbnails per session is still a possible follow-up if it bites in practice.Follow-ups
How to test
/captures/thumbnails/<id>/?label=largeon first load (direct storage URL once warm). Verified end-to-end on a local stack with the production portrait image and its nine real detections — screenshots above.python manage.py test ami.main.tests.TestImageThumbnailViews(29 tests, includes the two new ones).🤖 Generated with Claude Code