Conversation
✅ Deploy Preview for antenna-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for antenna-ssec ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a project summary dashboard displaying top identifiers, latest occurrences, and most observed taxa. The backend provides a new REST endpoint for user identification rankings, while the frontend introduces three data hooks and a refactored Summary component with nested overview sections, a new ListItem component for consistent item rendering, and internationalization support. ChangesProject Summary Dashboard
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
ami/main/api/views.py (1)
1869-1898: ⚡ Quick winRecommended: use
Count(filter=Q(...))and alignserializer_classwith the actual response shape.Two adjacent improvements:
- The aggregation works today because Django reuses the
identificationsjoin, but the canonical and safer pattern isCount("identifications", filter=Q(...)). It removes the asymmetry between the two branches (one usesdistinct=True, the other doesn't), drops the redundant trailing.distinct(), and is robust against future query changes.serializer_class = UserIdentificationCountSerializeris registered but the response is a hand-rolled{"project_id": ..., "top_identifiers": [...]}dict. drf-spectacular will document the inner item shape and miss the wrapper. Either build the response through a top-level serializer or declare it via@extend_schema(responses=...).♻️ Proposed simplification of the queryset
- # Start with user queryset - user_queryset = User.objects.all() - - # Filter by project if provided, then annotate with count - if project: - user_queryset = ( - user_queryset.filter(identifications__occurrence__project=project) - .annotate(identification_count=Count("identifications", distinct=True)) - .distinct() - ) - else: - user_queryset = user_queryset.annotate(identification_count=Count("identifications")) - - # Get top 5 users, ordered by identification count (descending) - top_identifiers = user_queryset.filter(identification_count__gt=0).order_by("-identification_count")[:5] + count_filter = ( + Q(identifications__occurrence__project=project) if project else Q() + ) + top_identifiers = ( + User.objects.annotate( + identification_count=Count("identifications", filter=count_filter, distinct=True) + ) + .filter(identification_count__gt=0) + .order_by("-identification_count")[:5] + )🤖 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/api/views.py` around lines 1869 - 1898, Refactor the aggregation in the top user queryset to use Count with a filter argument (Count("identifications", filter=Q(...))) for consistent and safer filtering instead of the current conditional annotations and distinct calls. Additionally, align the response with the declared serializer_class by either creating a top-level serializer that reflects the full response shape including "project_id" and "top_identifiers", or annotate the view with `@extend_schema` to explicitly declare the custom response structure to keep documentation in sync.
🤖 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.
Inline comments:
In `@ami/main/api/serializers.py`:
- Around line 1709-1718: The UserIdentificationCountSerializer currently exposes
email (email = serializers.CharField()) which leaks PII; remove the email field
from UserIdentificationCountSerializer so it matches UserNestedSerializer (id,
name, image, details) and update any UI/data consumers to use name/image only,
or alternatively keep the field but gate its inclusion in the view logic (e.g.,
only add email to the serialized output when request.user has project manager
permissions) by returning a different serializer or conditionally adding the
email in the view that constructs the response.
In `@ami/main/api/views.py`:
- Around line 1855-1905: The UserIdentificationCountsView exposes user PII such
as emails to anonymous users due to the broad IsActiveStaffOrReadOnly permission
and also lacks the require_project=True guard, allowing costly global
aggregations without project filtering. To fix this, set require_project = True
on the view to enforce project filtering, adjust permission_classes to a
stricter class like IsAuthenticated or IsProjectMember to prevent anonymous
access, and remove the email field from the response or restrict it so only
staff users receive it. These changes should be applied within the
UserIdentificationCountsView class.
In `@ui/src/pages/project/summary/list-item.tsx`:
- Around line 36-54: The img elements in the Image and UserImage components
currently use className="object-cover" but lack sizing so object-fit has no
effect; update the <img> in both Image and UserImage (functions/components named
Image and UserImage) to include full-size classes (e.g., add w-full and h-full
alongside object-cover) so the image fills the 12x12 container and object-cover
works as intended.
In `@ui/src/pages/project/summary/summary.tsx`:
- Around line 107-136: The "Latest occurrences" links are using an ascending
ordering query param; update both Link usages that build URLs via
APP_ROUTES.OCCURRENCE_DETAILS (inside occurrences.map) and
APP_ROUTES.OCCURRENCES (the "View all" link) to request descending order by
prefixing the field with a minus (e.g. ordering=-first_appearance_timestamp) so
the pages show most-recent-first to match useLatestOccurrences and the section
title.
- Around line 179-180: The Link pointing to APP_ROUTES.OCCURRENCES({ projectId
}) incorrectly appends "?verified=-true" (a malformed query) — update that Link
to use a proper boolean query (e.g., "?verified=false") so the URL is correct
and not brittle; locate the Link component referencing APP_ROUTES.OCCURRENCES
and projectId in summary.tsx and replace the "?verified=-true" fragment with a
valid query string such as "?verified=false" (or generate the query via
URLSearchParams) to fix the bug.
---
Nitpick comments:
In `@ami/main/api/views.py`:
- Around line 1869-1898: Refactor the aggregation in the top user queryset to
use Count with a filter argument (Count("identifications", filter=Q(...))) for
consistent and safer filtering instead of the current conditional annotations
and distinct calls. Additionally, align the response with the declared
serializer_class by either creating a top-level serializer that reflects the
full response shape including "project_id" and "top_identifiers", or annotate
the view with `@extend_schema` to explicitly declare the custom response structure
to keep documentation in sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e06b3030-6423-436b-9cfb-81575abff194
📒 Files selected for processing (12)
ami/main/api/serializers.pyami/main/api/views.pyconfig/api_router.pyui/src/components/error-state/error-state.tsxui/src/data-services/hooks/identifications/useTopIdentifiers.tsui/src/data-services/hooks/occurrences/useLatestOccurrences.tsui/src/data-services/hooks/species/useTopSpecies.tsui/src/pages/project/sidebar/sidebar.tsxui/src/pages/project/summary/list-item.tsxui/src/pages/project/summary/summary.tsxui/src/pages/species-details/species-details.tsxui/src/utils/language.ts
| class UserIdentificationCountSerializer(serializers.Serializer): | ||
| """ | ||
| Serializer for user identification counts. | ||
| """ | ||
|
|
||
| id = serializers.IntegerField() | ||
| name = serializers.CharField() | ||
| email = serializers.CharField() | ||
| image = serializers.CharField(required=False, allow_null=True) | ||
| identification_count = serializers.IntegerField() |
There was a problem hiding this comment.
email exposed to all project members — PII/compliance risk.
The existing UserNestedSerializer (line 62) deliberately omits email, exposing only id, name, image, and details. The new UserIdentificationCountSerializer breaks this pattern by adding email = serializers.CharField(), which the frontend renders directly for any user who can view the project summary (PR screenshot confirms the email is displayed in the "Most identifications" panel).
Exposing another user's email address to co-members without their consent is a GDPR/CCPA concern. Consider removing email from the serializer and relying on name/image for the UI card, or at minimum restricting the field to project managers in the view.
🤖 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/api/serializers.py` around lines 1709 - 1718, The
UserIdentificationCountSerializer currently exposes email (email =
serializers.CharField()) which leaks PII; remove the email field from
UserIdentificationCountSerializer so it matches UserNestedSerializer (id, name,
image, details) and update any UI/data consumers to use name/image only, or
alternatively keep the field but gate its inclusion in the view logic (e.g.,
only add email to the serialized output when request.user has project manager
permissions) by returning a different serializer or conditionally adding the
email in the view that constructs the response.
There was a problem hiding this comment.
Yes this is a good point, how do you think we should do with the main @mihow ...?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| class UserIdentificationCountsView(GenericAPIView, ProjectMixin): | ||
| """ | ||
| API endpoint that returns identification counts for top 5 users. | ||
| Optionally restricted to a specific project via project_id query parameter. | ||
| """ | ||
|
|
||
| permission_classes = [IsActiveStaffOrReadOnly] | ||
| serializer_class = UserIdentificationCountSerializer | ||
|
|
||
| @extend_schema(parameters=[project_id_doc_param]) | ||
| def get(self, request): | ||
| """ | ||
| Return top 5 users by identification count, optionally filtered by project. | ||
| """ | ||
| project = self.get_active_project() | ||
|
|
||
| # Start with user queryset | ||
| user_queryset = User.objects.all() | ||
|
|
||
| # Filter by project if provided, then annotate with count | ||
| if project: | ||
| user_queryset = ( | ||
| user_queryset.filter(identifications__occurrence__project=project) | ||
| .annotate(identification_count=Count("identifications", distinct=True)) | ||
| .distinct() | ||
| ) | ||
| else: | ||
| user_queryset = user_queryset.annotate(identification_count=Count("identifications")) | ||
|
|
||
| # Get top 5 users, ordered by identification count (descending) | ||
| top_identifiers = user_queryset.filter(identification_count__gt=0).order_by("-identification_count")[:5] | ||
|
|
||
| # Prepare serialized data | ||
| data = [] | ||
| for user in top_identifiers: | ||
| data.append( | ||
| { | ||
| "id": user.id, | ||
| "name": user.name if user.name else None, | ||
| "email": user.email, | ||
| "image": user.image.url if user.image else None, | ||
| "identification_count": user.identification_count, | ||
| } | ||
| ) | ||
|
|
||
| return Response( | ||
| { | ||
| "project_id": project.id if project else None, | ||
| "top_identifiers": data, | ||
| } | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify what IsActiveStaffOrReadOnly grants to anonymous users.
ast-grep --pattern $'class IsActiveStaffOrReadOnly($_) {
$$$
}'
rg -nP --type=py -C5 'class IsActiveStaffOrReadOnly\b'
# Also check whether other endpoints returning user PII apply stricter permissions:
rg -nP --type=py -C2 '\buser\.email\b|"email":\s*user\.email'Repository: RolnickLab/antenna
Length of output: 9674
🏁 Script executed:
#!/bin/bash
# Get full IsActiveStaffOrReadOnly implementation
rg -nA20 'class IsActiveStaffOrReadOnly' ami/base/permissions.pyRepository: RolnickLab/antenna
Length of output: 876
🏁 Script executed:
#!/bin/bash
# Find SummaryView and check require_project setting
rg -nA30 'class SummaryView' ami/main/api/views.py | head -50Repository: RolnickLab/antenna
Length of output: 1800
🏁 Script executed:
#!/bin/bash
# Show the full UserIdentificationCountsView implementation focusing on Count logic
sed -n '1855,1905p' ami/main/api/views.py | cat -nRepository: RolnickLab/antenna
Length of output: 2312
🏁 Script executed:
#!/bin/bash
# Check if there are tests for UserIdentificationCountsView
rg -n 'UserIdentificationCountsView|user.*identification.*count' ami/main/tests.py ami/main/api/tests/ --type=py 2>/dev/null | head -20Repository: RolnickLab/antenna
Length of output: 44
Major: PII exposure risk and missing require_project guard.
Two confirmed concerns on this endpoint:
-
PII exposure (emails) under
IsActiveStaffOrReadOnly. This permission allows read access to anonymous users (anyone can GET). The view returnsemailandnamefor the top 5 users, exposing personally identifiable information to unauthenticated callers. The neighbouringSummaryViewuses the same permission but returns only aggregated counts—no user details. -
No
require_projectguard; unbounded global aggregation.SummaryViewsetsrequire_project = Truewith a comment: "Unfiltered summary aggregates are too expensive". This view'selsebranch runsUser.objects.annotate(identification_count=Count("identifications"))without a project filter, triggering a full table scan on theIdentificationtable. An anonymous caller can trigger this expensive operation.
Add require_project = True and consider tightening the permission class (e.g., IsAuthenticated or IsProjectMember) and/or omitting email from the response for non-staff callers.
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 1861-1861: Mutable default value for class attribute
(RUF012)
🤖 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/api/views.py` around lines 1855 - 1905, The
UserIdentificationCountsView exposes user PII such as emails to anonymous users
due to the broad IsActiveStaffOrReadOnly permission and also lacks the
require_project=True guard, allowing costly global aggregations without project
filtering. To fix this, set require_project = True on the view to enforce
project filtering, adjust permission_classes to a stricter class like
IsAuthenticated or IsProjectMember to prevent anonymous access, and remove the
email field from the response or restrict it so only staff users receive it.
These changes should be applied within the UserIdentificationCountsView class.
There was a problem hiding this comment.
For the optional project, I setup the endpoint so we could also use it for top identifiers across all projects in future, however this is not used yet. Do you think it will be too heavy @mihow ?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
1 participant
Summary
In this PR we include some live data in the project summary. This is to make the project dashboard feel a bit more alive and interactive. The information about top identifiers is new and not something we have been able to see before. Maybe this could be motivating for team members to see?
List of Changes
Detailed Description
How to Test the Changes
@mihow could you help test and review the backend changes I included, since I'm no experts here?
Screenshots
Next Steps
Include a stats section in the occurrence view where we can see total number of identifications, percentage of occurrences verified and maybe even human-model agreement rate?
Summary by CodeRabbit
Release Notes
New Features
Style
Chores