Add live data to project summary#1296
Merged
Merged
Conversation
✅ Deploy Preview for antenna-preview canceled.
|
✅ Deploy Preview for antenna-ssec canceled.
|
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR introduces a complete top-identifiers stats endpoint with a project summary dashboard. The backend adds aggregation logic, serializers, and a DRF viewset action; the frontend implements data hooks and refactors the summary page with three new widgets; and supporting translations, styling updates, and comprehensive documentation complete the feature. ChangesProject Summary Dashboard with Top Identifiers API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 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 unit tests (beta)
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}
Contributor
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
mihow
reviewed
May 13, 2026
- Drop `email` from UserIdentificationCountSerializer + view response. Existing UserNestedSerializer omits email for the same PII reasons; match that contract. UI now renders name only. - Refactor UserIdentificationCountsView to use serializer_class + get_serializer(...) (was hand-rolling response dict). - Set `require_project = True`. Avoids unbounded global aggregation on the Identification table (mirrors SummaryView's guard). - Use `Count(filter=Q(...))` so the annotation only counts a user's identifications scoped to the requested project, not their global total (was returning the same number from the JOIN-then-count form). - list-item.tsx: `text` becomes optional, `<img>` gets `w-full h-full object-cover` so cropping works inside the 12x12 box. Co-Authored-By: Claude <noreply@anthropic.com>
UserIdentificationCountsView returned identifier names + profile photos for any project_id, including draft projects. Pattern matches CodeRabbit's concern about anonymous read access on this view. Add a Project.visible_for_user(request.user) check that raises 404 when the requested project is a draft and the caller is not a superuser, owner, or member. Anonymous + non-member callers can no longer enumerate contributors on unpublished projects. Co-Authored-By: Claude <noreply@anthropic.com>
Previously `Count("identifications", distinct=True)` counted raw
Identification rows, so a user who corrected their own ID on the same
occurrence had their leaderboard number inflated by every revision.
Switch to `Count("identifications__occurrence", distinct=True)` so the
metric matches the panel's intent: number of distinct occurrences the
user has identified in the project.
Verified against local fixtures: counts for users with duplicate IDs on
the same occurrence drop to the deduplicated value (e.g. 7 → 5),
unaffected users stay flat.
Co-Authored-By: Claude <noreply@anthropic.com>
LatestOccurrences, MostIdentifications, and MostObservedTaxa shared a near-identical loading/error/empty/list+ViewAll skeleton. Factor that out into a SummaryColumn shell that takes the load state and a viewAllHref, and have each column pass its rows as children. Adds a single seam for the upcoming fourth stats tile rather than a fourth copy. Drop the dead `?ordering=-...` querystrings on per-row OCCURRENCE_DETAILS and TAXON_DETAILS links: detail pages don't run `useSort`, and the list context is re-derived from the list URL on back-navigation, so the querystring was inert noise. ViewAll links keep `?ordering=` since those target list pages that honor it. Co-Authored-By: Claude <noreply@anthropic.com>
…ers migration Captures the design decision (entity-rooted + stats namespace, scope via query, M2 colocated module structure) and the 3-commit implementation plan to migrate /users/identifications/top/ to /occurrences/stats/top-identifiers/ before merging PR #1296. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/src/pages/project/summary/summary.tsx (1)
1-270:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix blocking formatting check in this file.
yarn format --checkis failing onui/src/pages/project/summary/summary.tsx, so this PR won’t pass frontend lint until this file is reformatted with the project’s pinned formatter.Based on learnings: In the
ui/project (RolnickLab/antenna), formatting must be produced with the project-local Prettier pinned to 2.8.4 (use repo-local Prettier, not a global version).🤖 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/project/summary/summary.tsx` around lines 1 - 270, The file fails the repository's formatting check; reformat ui/src/pages/project/summary/summary.tsx using the repo-local Prettier (pinned to 2.8.4) so the frontend lint passes. Locate the Summary component and its helper components (LatestOccurrences, MostIdentifications, MostObservedTaxa, Charts, SummaryColumn) and run the project-local formatter (e.g. the repo's yarn format script or npx prettier@./node_modules/prettier --write) to apply the canonical formatting, then re-run yarn format --check before committing.
🧹 Nitpick comments (1)
docs/claude/planning/pr-1296-occurrence-stats-migration.md (1)
188-188: ⚡ Quick winAvoid machine-specific absolute paths in committed runbooks.
/home/michael/...is not portable for teammates and leaks local workstation details. Prefer repo-relative references or a placeholder variable.Suggested patch
-2. Stack still bind-mounted into main compose (`/home/michael/Projects/AMI/antenna/docker-compose.override.yml` overrides django+celeryworker+ui-dev to point at worktree) +2. Stack still bind-mounted into main compose (use your local `docker-compose.override.yml`; it overrides django+celeryworker+ui-dev to point at the worktree)🤖 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/pr-1296-occurrence-stats-migration.md` at line 188, Replace the machine-specific absolute path "/home/michael/Projects/AMI/antenna/docker-compose.override.yml" in the runbook with a portable reference: either a repo-relative path (e.g., ./docker-compose.override.yml) or a placeholder variable (e.g., {WORKTREE_DOCKER_COMPOSE_OVERRIDE}) and include a short note explaining how to resolve it locally; update the occurrence in the planning doc (the line that currently mentions the absolute path) so teammates can use a repo-relative or placeholder value instead of leaking a local workstation path.
🤖 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 `@docs/claude/planning/pr-1296-occurrence-stats-migration.md`:
- Around line 22-29: The fenced code block containing the HTTP status lines (the
triple-backtick block that starts immediately before "/users/
HTTP 401 djoser owns this") needs a language label to satisfy MD040; update
the opening fence from ``` to ```text (or another appropriate language) so the
block becomes a labeled code fence and linting passes.
- Around line 105-106: The current code directly casts
request.query_params.get("limit", 5) to int which raises on invalid strings;
update the handler to validate the limit by attempting the int conversion in a
try/except (catch ValueError/TypeError) and return a 400 Bad Request with a
clear error message when conversion fails, otherwise proceed to clamp the parsed
integer (limit = max(1, min(limit, 50))). Locate uses of
request.query_params.get("limit", 5) and the variable limit to implement this
validation and error response.
---
Outside diff comments:
In `@ui/src/pages/project/summary/summary.tsx`:
- Around line 1-270: The file fails the repository's formatting check; reformat
ui/src/pages/project/summary/summary.tsx using the repo-local Prettier (pinned
to 2.8.4) so the frontend lint passes. Locate the Summary component and its
helper components (LatestOccurrences, MostIdentifications, MostObservedTaxa,
Charts, SummaryColumn) and run the project-local formatter (e.g. the repo's yarn
format script or npx prettier@./node_modules/prettier --write) to apply the
canonical formatting, then re-run yarn format --check before committing.
---
Nitpick comments:
In `@docs/claude/planning/pr-1296-occurrence-stats-migration.md`:
- Line 188: Replace the machine-specific absolute path
"/home/michael/Projects/AMI/antenna/docker-compose.override.yml" in the runbook
with a portable reference: either a repo-relative path (e.g.,
./docker-compose.override.yml) or a placeholder variable (e.g.,
{WORKTREE_DOCKER_COMPOSE_OVERRIDE}) and include a short note explaining how to
resolve it locally; update the occurrence in the planning doc (the line that
currently mentions the absolute path) so teammates can use a repo-relative or
placeholder value instead of leaking a local workstation path.
🪄 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: 89457e85-a1c0-4f6b-ab8f-e302be4c9a8b
📒 Files selected for processing (3)
ami/main/api/views.pydocs/claude/planning/pr-1296-occurrence-stats-migration.mdui/src/pages/project/summary/summary.tsx
Re-review pass on 2026-05-13 corrected three factual claims and locked open design questions: Corrections - "djoser-squat" claim was wrong; explicit path() runs before router.urls so the current /users/identifications/top/ route works. Real motivation to migrate: bypasses router (no autodoc grouping) + pollutes /users/ hierarchy - "/occurrences/stats/ clean greenfield" was wrong; falls into OccurrenceViewSet.retrieve(pk="stats"). Registration order is mandatory, not "verify at apply time" - Tests promoted from "open question" to required scope in Commit 2 Locked decisions - Nested r"occurrences/stats" over sibling r"occurrence-stats" (hierarchy) - Noun = stats not metrics (avoid Prometheus/ops connotation) - Response shapes vary per kind, always declared via DRF serializer; envelope kept for top-identifiers to match existing FE shape (no FE churn) - limit param uses SingleParamSerializer with min_value=1/max_value=50, strict 400 on out-of-range (matches ami/jobs/views.py:321 precedent) - FE hook moves to ui/src/data-services/hooks/occurrences/stats/ - get_active_project() discipline documented in viewset docstring - Pydantic ruled out — DRF serializer integrates with drf-spectacular natively Plan now has 4 commits (Commit 4 = convention reference doc at docs/claude/reference/api-stats-pattern.md) so future stats endpoints have a target to point at. Co-Authored-By: Claude <noreply@anthropic.com>
Saves the original PR description verbatim so post-compact session can update the PR body without clobbering Anna's writing. Anna remains the author; new commits append a backend-follow-ups section, never replace existing content. Co-Authored-By: Claude <noreply@anthropic.com>
No behavior change. UserIdentificationCountsView now calls top_identifiers_for_project() from ami/main/models_future/occurrence.py. Sets up Commit 2: a new /occurrences/stats/top-identifiers/ endpoint calls the same function. Co-Authored-By: Claude <noreply@anthropic.com>
Establishes the /<entity>/stats/<kind>/ pattern as the convention for
aggregate stats endpoints (see plan doc at
docs/claude/planning/pr-1296-occurrence-stats-migration.md; permanent
reference will land in Commit 4).
New endpoint mirrors the legacy /users/identifications/top/ envelope
exactly so the frontend swap in Commit 3 is just a URL change. The
legacy route stays live until Commit 3 deletes it.
- OccurrenceStatsViewSet on a ViewSet (not GenericViewSet) — actions are
independent aggregations, no shared queryset
- TopIdentifiersResponseSerializer declares the envelope so spectacular
can autodoc it (no raw Response({...}) shapes allowed in this viewset)
- limit param via SingleParamSerializer[int] with min/max validation —
strict 400 on out-of-range, matches ami/jobs/views.py:321 precedent
- r"occurrences/stats" MUST register BEFORE r"occurrences"; the regex
^occurrences/(?P<pk>[^/.]+)/$ would otherwise capture stats/ first.
Comment in api_router.py + regression test guard against accidental
re-ordering.
Tests (8 in TestOccurrenceStatsTopIdentifiers):
- happy_path ranks users by distinct occurrences
- limit param caps results
- limit below min / above max → 400
- missing project_id → 400 (via ProjectMixin.require_project)
- draft project → 404 for anon (visible_for_user gate)
- distinct count: revising your own ID on the same occurrence = 1, not 2
- registration_order: /occurrences/<pk>/ retrieve still resolves
Co-Authored-By: Claude <noreply@anthropic.com>
Completes the migration started in Commit 2: - Move the frontend hook to ui/src/data-services/hooks/occurrences/stats/ (mirrors the backend URL hierarchy) - Flip the hook URL to /occurrences/stats/top-identifiers/ - Delete UserIdentificationCountsView class + its path() registration at /users/identifications/top/ Response envelope is unchanged, so no UI logic changes — just the URL. Co-Authored-By: Claude <noreply@anthropic.com>
Permanent reference doc for the pattern established by OccurrenceStatsViewSet.top_identifiers. Documents the registration-order trap, the stats-vs-metrics noun choice, response-shape rules, SingleParamSerializer usage, the get_active_project() discipline, the models_future split, FE hook layout, and the test floor for new stats actions. Co-Authored-By: Claude <noreply@anthropic.com>
…view # Conflicts: # ami/main/models_future/occurrence.py
mihow
reviewed
May 14, 2026
Collaborator
|
Claude says: Establishing a new convention with this PR: Live example: This is where future product-domain aggregates will land:
Conventions enforced for any new stats action:
Convention reference:
|
…ies) Adds /deployments/stats/ examples (processed images per station, capture status counts) and additional /occurrences/stats/ examples (per-species identification counts, model agreement rate) to the convention doc, so future PRs touching those use cases land on the established pattern. Co-Authored-By: Claude <noreply@anthropic.com>
mihow
reviewed
May 14, 2026
mihow
reviewed
May 14, 2026
Addresses mihow's review feedback on /occurrences/stats/top-identifiers/:
ride the standard list-endpoint rails instead of a hand-rolled envelope.
Also demonstrates the scalar/chart shape on the same viewset so the
convention covers both kinds.
Backend
- OccurrenceStatsViewSet now inherits viewsets.GenericViewSet with
pagination_class=StatsPagination (default_limit=5, max_limit=50) and
filter_backends=[NullsLastOrderingFilter] + ordering_fields=
["identification_count"]. Per-action: call paginate_queryset() +
get_paginated_response() for list-shaped kinds; build dict +
Response(serializer.data) for scalar/chart kinds.
- top_identifiers: drop inner [:limit], drop SingleParamSerializer for
limit, drop {project_id, top_identifiers: [...]} envelope. Response
is now DRF's {count, next, previous, results: [...]}.
?limit=N, ?offset=M, ?ordering=-identification_count all work.
- identifications_summary: new scalar placeholder action demonstrating
pattern 2 (serializer-declared shape, no paginator). Returns
{total_identifications, distinct_identifiers,
distinct_identified_occurrences}. Real consumers (verification rate,
model agreement, etc) follow the same shape.
- top_identifiers_for_project: drop limit param (paginator handles it).
Keep .filter(identification_count__gt=0) as non-configurable — anon
/ empty calls must never leak the full project user list, regardless
of how the caller slices.
- IdentificationsSummarySerializer added; TopIdentifiersResponseSerializer
deleted (paginator wraps automatically).
Tests
- TestOccurrenceStatsTopIdentifiers → TestOccurrenceStatsViewSet
(12 tests covering both patterns + shared regression).
- New: default_limit_is_five, ordering_param_overrides_default,
excludes_users_with_zero_count, summary scalar happy path,
summary missing project, summary draft 404.
- Dropped: limit_below_min/above_max 400 cases — paginator silently
falls back to default on invalid values, doesn't 400.
- registration_order test now covers both stats subroutes.
Frontend
- useTopIdentifiers reads {count, results: [...]} and accepts an
optional limit arg (defaults to 5, passed as ?limit=5 explicitly).
- MostIdentifications consumer reads data?.results.
Docs
- api-stats-pattern.md rewritten to cover both response shapes, the
"don't leak entity lists" rule for list-kinds, and updated future
examples split by pattern.
Co-Authored-By: Claude <noreply@anthropic.com>
Drop the placeholder `identifications_summary` action and the paginator/filter-backend machinery added in 4c9acd5. The leaderboard is intentionally short (default 5, max 50) — `{count, next, previous}` fields per response add overhead with no consumer benefit, and the placeholder existed only to demonstrate a scalar shape the leaderboard itself wanted. Changes: - `OccurrenceStatsViewSet` returns `{project_id, top_identifiers: [...]}` via `TopIdentifiersResponseSerializer`. One action, scalar shape. - `_gate_project` helper inlined (2 lines, single call site). - `?limit` validated via `SingleParamSerializer[int].clean()` — strict 400 on out-of-range instead of silent paginator clamp. - `identification_count >= 1` filter stays baked into `top_identifiers_for_project` (non-configurable). - Tests trimmed 13 → 6 (drop pagination/ordering/summary tests, fold default_limit + distinct-count invariants into happy path). - FE hook + `summary.tsx` read `data?.top_identifiers` instead of `data?.results`. - Reference doc rewritten scalar-first. - New planning doc captures the paginator design ready to revive when a stats kind genuinely needs `?limit / ?offset / ?ordering` rails on hundreds of rows. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
docs/claude/reference/api-stats-pattern.md (2)
77-77: 💤 Low valueFix grammar: "needs paginated" → "needs to be paginated" or "needs pagination".
The phrase "needs paginated" is nonstandard in formal writing.
📝 Suggested fix
-When a kind genuinely needs paginated rails (e.g. a deployment +When a kind genuinely needs to be paginated (e.g. a deploymentor
-When a kind genuinely needs paginated rails (e.g. a deployment +When a kind genuinely needs pagination (e.g. a deployment🤖 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/reference/api-stats-pattern.md` at line 77, Replace the nonstandard phrase "needs paginated" in the sentence fragment "When a kind genuinely needs paginated rails (e.g. a deployment" with correct wording — either "needs to be paginated" or "needs pagination" — so the sentence reads e.g. "When a kind genuinely needs to be paginated (e.g. a deployment...)" or "When a kind genuinely needs pagination (e.g. a deployment...)".
191-191: 💤 Low valueAdd language identifier to fenced code block for linting compliance.
The code block showing file paths should have a language identifier to satisfy markdown linting rules.
📝 Suggested fix
-``` +```text ui/src/data-services/hooks/<entity>/stats/<kind>.ts</details> <details> <summary>🤖 Prompt for AI Agents</summary>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/reference/api-stats-pattern.mdat line 191, The fenced code
block containing the path string
"ui/src/data-services/hooks//stats/.ts" is missing a language
identifier; update the opening fence fromtotext so the block is marked
as plain text (e.g., replace the opening triple backticks with ```text) to
satisfy markdown linting.</details> </blockquote></details> <details> <summary>docs/claude/planning/pr-1296-occurrence-stats-migration.md (1)</summary><blockquote> `250-250`: _💤 Low value_ **Add period after "etc" for American English style.** The abbreviation "etc" should be "etc." in American English conventions. <details> <summary>📝 Suggested fix</summary> ```diff -2. Next stats endpoint that lands (species-accuracy, processing-progress, etc) confirms the +2. Next stats endpoint that lands (species-accuracy, processing-progress, etc.) confirms the ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/pr-1296-occurrence-stats-migration.md` at line 250, Update the sentence containing the phrase "species-accuracy, processing-progress, etc" to use American English punctuation by adding a period after "etc" (change "etc" to "etc."). Locate the exact occurrence of that phrase in the document and replace it so the text reads "species-accuracy, processing-progress, etc." ensuring only the punctuation is changed. ``` </details> </blockquote></details> <details> <summary>ami/main/api/views.py (1)</summary><blockquote> `1306-1309`: _💤 Low value_ **Prefer an explicit guard over `assert` for the project-presence invariant.** `assert project is not None` is stripped under `python -O`. The comment "require_project=True guarantees this" treats it as documentation, but if a future change to `ProjectMixin` ever returns `None` here, the next-line `project.pk` access would raise `AttributeError` rather than a clean 400/404. Since you're also doing a visibility lookup right after, an explicit guard costs nothing. <details> <summary>🛡️ Proposed change</summary> ```diff - project = self.get_active_project() - assert project is not None # require_project=True guarantees this - if not Project.objects.visible_for_user(request.user).filter(pk=project.pk).exists(): + project = self.get_active_project() + if project is None or not Project.objects.visible_for_user(request.user).filter(pk=project.pk).exists(): raise NotFound("Project not found.") ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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 1306 - 1309, Replace the assert with an explicit guard: check the value returned by get_active_project() (the local variable project) and if it's None raise NotFound (or another appropriate HTTP exception) before accessing project.pk; then proceed with the existing Project.objects.visible_for_user(...) visibility check. This removes reliance on assert and ensures a clean 404 if get_active_project() ever returns None. ``` </details> </blockquote></details> <details> <summary>ami/main/api/schemas.py (1)</summary><blockquote> `10-15`: _💤 Low value_ **Description hardcodes per-action bounds in a shared parameter.** The description embeds `(1-50, default 5)`, but those constraints live in the `top_identifiers` action's `IntegerField(min_value=1, max_value=50, default=5)`. If this `OpenApiParameter` is intended to be reused across future stats actions (per the convention in `docs/claude/reference/api-stats-pattern.md`), other actions with different bounds will either silently mismatch or need their own `OpenApiParameter`. Consider keeping the description generic and declaring action-specific bounds inline in `@extend_schema` where they differ. <details> <summary>📝 Proposed change</summary> ```diff limit_doc_param = OpenApiParameter( name="limit", - description="Maximum number of items to return (1-50, default 5).", + description="Maximum number of items to return. Per-action bounds and default apply.", required=False, type=int, ) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/schemas.py` around lines 10 - 15, The shared OpenApiParameter limit_doc_param currently hardcodes "(1-50, default 5)" which conflicts with action-specific bounds like top_identifiers' IntegerField(min_value=1, max_value=50, default=5); remove the per-action numeric bounds from limit_doc_param's description and make it generic (e.g. "Maximum number of items to return.") and then declare action-specific bounds and defaults where they belong (for example in the top_identifiers view/serializer IntegerField or in that action's `@extend_schema` parameter override) so each endpoint documents its own min_value/max_value/default consistently. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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/tests.py:
- Around line 4697-4705: In setUp(), guard against a nullable self.taxon
returned by Taxon.objects.filter(projects=project).first() by asserting it is
not None (e.g., using assertIsNotNone or an explicit check) so test failures are
immediate and descriptive; update the setUp() method where self.taxon is
assigned and before returning to raise or assert with a clear message like "No
Taxon found for project" so calls to _id(user, occurrence) and
Identification.objects.create(...) won't fail with an obscure DB error.
Nitpick comments:
In@ami/main/api/schemas.py:
- Around line 10-15: The shared OpenApiParameter limit_doc_param currently
hardcodes "(1-50, default 5)" which conflicts with action-specific bounds like
top_identifiers' IntegerField(min_value=1, max_value=50, default=5); remove the
per-action numeric bounds from limit_doc_param's description and make it generic
(e.g. "Maximum number of items to return.") and then declare action-specific
bounds and defaults where they belong (for example in the top_identifiers
view/serializer IntegerField or in that action's@extend_schemaparameter
override) so each endpoint documents its own min_value/max_value/default
consistently.In
@ami/main/api/views.py:
- Around line 1306-1309: Replace the assert with an explicit guard: check the
value returned by get_active_project() (the local variable project) and if it's
None raise NotFound (or another appropriate HTTP exception) before accessing
project.pk; then proceed with the existing Project.objects.visible_for_user(...)
visibility check. This removes reliance on assert and ensures a clean 404 if
get_active_project() ever returns None.In
@docs/claude/planning/pr-1296-occurrence-stats-migration.md:
- Line 250: Update the sentence containing the phrase "species-accuracy,
processing-progress, etc" to use American English punctuation by adding a period
after "etc" (change "etc" to "etc."). Locate the exact occurrence of that phrase
in the document and replace it so the text reads "species-accuracy,
processing-progress, etc." ensuring only the punctuation is changed.In
@docs/claude/reference/api-stats-pattern.md:
- Line 77: Replace the nonstandard phrase "needs paginated" in the sentence
fragment "When a kind genuinely needs paginated rails (e.g. a deployment" with
correct wording — either "needs to be paginated" or "needs pagination" — so the
sentence reads e.g. "When a kind genuinely needs to be paginated (e.g. a
deployment...)" or "When a kind genuinely needs pagination (e.g. a
deployment...)".- Line 191: The fenced code block containing the path string
"ui/src/data-services/hooks//stats/.ts" is missing a language
identifier; update the opening fence fromtotext so the block is marked
as plain text (e.g., replace the opening triple backticks with ```text) to
satisfy markdown linting.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `47dfdc57-35cc-4954-84fe-04825acf34dd` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 86d2f90117d1e2b7f2efb369f02d109fb1111796 and fbf43826bbb7295e0662580cf3287e7a273dc983. </details> <details> <summary>📒 Files selected for processing (12)</summary> * `ami/main/api/schemas.py` * `ami/main/api/serializers.py` * `ami/main/api/views.py` * `ami/main/models_future/occurrence.py` * `ami/main/tests.py` * `config/api_router.py` * `docs/claude/planning/pr-1296-occurrence-stats-migration.md` * `docs/claude/planning/stats-list-pattern.md` * `docs/claude/reference/api-stats-pattern.md` * `docs/claude/sessions/pr-1296-anna-original-body.md` * `ui/src/data-services/hooks/occurrences/stats/useTopIdentifiers.ts` * `ui/src/pages/project/summary/summary.tsx` </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * ui/src/pages/project/summary/summary.tsx </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
The 3-line explanation duplicated docs/claude/reference/api-stats-pattern.md and the regression test (test_registration_order_preserves_occurrence_retrieve) already guards the invariant. Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
|
Claude says: Hi @annavik — quick summary of FE changes so you can review with focus. Untouched:
Backend grew (stats endpoint convention, |
Plan doc served its purpose — all four planned commits landed (plus a paginator detour that was reverted). Live convention reference at docs/claude/reference/api-stats-pattern.md is the canonical pointer for future stats endpoints; archive preserves the design-decision audit trail. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/claude/archive/pr-1296-occurrence-stats-migration.md (1)
22-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language identifier to the fenced code block.
Line 22 opens an unlabeled fenced block, which still triggers MD040 markdown lint. Use
```textfor this status-list block.Suggested patch
-``` +```text /users/... handled by djoser router (registered as r"users") /users/identifications/top/ handled by explicit path() listed BEFORE router.urls → works today because Django URL resolver matches in order; not actually claimed by djoser, but bypasses the router → pollutes /users/ hierarchy + no autodoc grouping /identifications/ our IdentificationViewSet /occurrences/ our OccurrenceViewSet /occurrences/stats/ currently dispatches to OccurrenceViewSet.retrieve(pk="stats") → 404 via Occurrence.objects.get(pk="stats") ValueError. NOT clean greenfield — slot must be claimed by registering r"occurrences/stats" BEFORE r"occurrences"</details> <details> <summary>🤖 Prompt for AI Agents</summary>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/archive/pr-1296-occurrence-stats-migration.mdaround lines 22 -
34, The fenced status-list block starting withmust include a language identifier to satisfy MD040; change the opening fence fromtotext so the block begins with "text" and leave the rest of the list content unchanged
(the lines describing /users/..., /users/identifications/top/,
/identifications/, /occurrences/, /occurrences/stats/).</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Duplicate comments:
In@docs/claude/archive/pr-1296-occurrence-stats-migration.md:
- Around line 22-34: The fenced status-list block starting with
must include a language identifier to satisfy MD040; change the opening fence fromto
text so the block begins with "text" and leave the rest of the list
content unchanged (the lines describing /users/..., /users/identifications/top/,
/identifications/, /occurrences/, /occurrences/stats/).</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `96e107c1-3662-4944-96c3-3dc4c88227c5` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between fbf43826bbb7295e0662580cf3287e7a273dc983 and 1da2943420d3c0b755cf00c73b5dfd282190627f. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `config/api_router.py` * `docs/claude/archive/pr-1296-occurrence-stats-migration.md` </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * config/api_router.py </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
The previous archive-only commit used [no ci]; force a run so the scalar-shape FE revert (fbf4382) gets verified on CI before merge. Co-Authored-By: Claude <noreply@anthropic.com>
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
2 participants
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
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
Introduces an API pattern for adding stats & aggregate summary metrics around the app:
In this example:
GET /api/v2/occurrences/stats/top-identifiers/?project_id=18&limit=5Other upcoming data that might use this:
/occurrences/stats/identifications-by-species/— per-taxon identification counts/occurrences/stats/human-model-agreement/— model agreement / verification rate/deployments/stats/processed-images/— processed images per station or capture counts per processing state/taxa/stats/model-accuracy/How to Test the Changes
Backend extended past the original prototype — see "Backend follow-ups" below for the convention and verification notes.
Screenshots
After removing emails:
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?
Backend follow-ups (Claude commits)
Anna's "could @mihow help review the backend" ask was picked up — the original prototype was extended into a reusable convention so future stats endpoints land in the same namespace.
Endpoint (legacy
/users/identifications/top/removed, no overlap period):GET /occurrences/stats/top-identifiers/?project_id=X[&limit=N]→{project_id, top_identifiers: [{id, name, image, identification_count}, ...]}Convention:
/api/v2/<entity>/stats/<kind>/— full writeup atdocs/claude/reference/api-stats-pattern.md. Each kind owns its response shape (scalar by default; opt into the DRF paginator envelope only when a kind genuinely needs?limit / ?offset / ?orderingrails on hundreds of rows — deferred design atdocs/claude/planning/stats-list-pattern.md).Design notes:
OccurrenceStatsViewSetis aviewsets.GenericViewSet+ProjectMixin, one@actionper stats kind.ami/main/models_future/occurrence.py::top_identifiers_for_project(). Bakes inidentification_count >= 1non-configurably — anon / empty calls never leak the project user list.?limitvalidated viaSingleParamSerializer[int].clean()— 400 on out-of-range instead of silent clamp.Project.objects.visible_for_user(request.user));ProjectMixinenforces presence (400/404 on missing/unknown id) but not draft filtering.r"occurrences/stats"MUST register beforer"occurrences"— guarded byTestOccurrenceStatsViewSet.test_registration_order_preserves_occurrence_retrieve.Verification:
python manage.py test ami.main.tests.TestOccurrenceStatsViewSet --keepdb→ 6/6 passConvention reference:
docs/claude/reference/api-stats-pattern.md.Summary by CodeRabbit
Release Notes
New Features
Style
Chores