fix: address taxa-list PR review feedback by mihow · Pull Request #1119 · RolnickLab/antenna

mihow · 2026-02-05T07:23:57Z

Summary

Addresses review feedback (mihow, Copilot, CodeRabbit) on the taxa-lists feature branch. Grouped into six logical commits.

Bug fixes

entity.ts — createdAt getter was guarding on updated_at instead of created_at
utils.ts — reverted project_id back to project; existing serializers (Device, Site, StorageSource, SourceImageCollection) all declare the field as project
useTaxaListDetails.ts — generic type changed from TaxaList (client shape) to ServerTaxaList (raw API shape); added projectId to query key to prevent cross-project cache collisions
taxa-list-details.tsx — Table isLoading was gated on !id which is always falsy (route param), so loading state never showed

Security / permissions

Added IsProjectMemberOrReadOnly permission class (ami/base/permissions.py). Safe methods are open; unsafe methods (POST/DELETE) require the user to be a member of the active project. Scoped tighter than IsActiveStaffOrReadOnly (which blocks non-staff project members) and looser than guardian object-level perms (which don't cover M2M-to-project models like TaxaList).
Applied to TaxaListTaxonViewSet — was permission_classes = [] (completely open).

Backend cleanup

TaxonTaxaListFilter docstring corrected: include_descendants defaults to true, not false
get_taxa_list: added from None to suppress chained DoesNotExist traceback
create: replaced Taxon.objects.get with get_object_or_404 to close the race between serializer validation and the lookup

UX

"Add taxon" button in header now gated on canUpdate (the per-row remove button already was)
Add button disabled while the add mutation is in flight
Remove confirm button disabled while the remove mutation is in flight
Taxa-lists page result count uses the paginated total from the API instead of current-page length

Naming / dead code

Deduplicated MESSAGE_MESSAGE_REMOVE_TAXA_LIST_TAXON_CONFIRM → MESSAGE_REMOVE_TAXA_LIST_TAXON_CONFIRM
Removed unused API_ROUTES import in new-entity-dialog.tsx
Removed dead expression in useSidebarSections.tsx (computed value never used)
Added missing setMainBreadcrumb to useEffect dep array in breadcrumbs.tsx

Accessibility

Three dialog sites (taxa-list-details, species, occurrences) called e.preventDefault() in onOpenAutoFocus without transferring focus. Added explicit .focus() on the dialog content in each.

Test plan

ami.main.tests.TaxaListTaxonAPITestCase and TaxaListTaxonValidationTestCase — all 13 pass with non-staff project-member users
Manually: create a taxa list as a non-staff project member, add and remove taxa
Manually: confirm a non-member cannot POST/DELETE on the taxa-list taxon endpoint
Visually confirm the "Add taxon" button disappears for read-only users
Visually confirm Add and Remove buttons stay disabled while their requests are in flight

…e and cache key, table loading state - entity.ts: createdAt getter was guarding on updated_at instead of created_at - utils.ts: revert project_id back to project; existing serializers (Device, Site, StorageSource, SourceImageCollection) declare the field as project - useTaxaListDetails: generic was TaxaList (client shape) instead of ServerTaxaList (raw API shape); added projectId to queryKey to avoid cross-project cache hits - taxa-list-details: isLoading on Table was gated on !id which is always falsy (id is a route param), so the loading state never showed Co-Authored-By: Claude <noreply@anthropic.com>

… tests TaxaListTaxonViewSet had permission_classes = [] (open to everyone). Set to IsActiveStaffOrReadOnly to match the parent TaxaListViewSet pattern. Test setUp now creates users with is_staff=True so POST/DELETE pass the permission check. Co-Authored-By: Claude <noreply@anthropic.com>

- TaxonTaxaListFilter docstring: include_descendants default was documented as false but the code defaults to True; corrected docs to match - get_taxa_list: added "from None" to suppress chained DoesNotExist traceback in the NotFound exception - create: replaced Taxon.objects.get with get_object_or_404 to close the race between serializer validation and the lookup - added get_object_or_404 import Co-Authored-By: Claude <noreply@anthropic.com>

- taxa-list-details: wrap AddTaxaListTaxonPopover in canUpdate check so users without write permission do not see the add button - add-taxa-list-taxon: disable the Add button while the mutation is in flight to prevent duplicate submissions - remove-taxa-list-taxon-dialog: disable the Confirm button while the remove mutation is in flight - taxa-lists: use the paginated total from the API instead of the length of the current page for the results count Co-Authored-By: Claude <noreply@anthropic.com>

- language.ts: rename MESSAGE_MESSAGE_REMOVE_TAXA_LIST_TAXON_CONFIRM to MESSAGE_REMOVE_TAXA_LIST_TAXON_CONFIRM (duplicated prefix); update enum key, translation map entry, and the one usage site - new-entity-dialog.tsx: remove unused API_ROUTES import - useSidebarSections.tsx: remove expression that computes a value but never assigns or returns it - breadcrumbs.tsx: add setMainBreadcrumb to useEffect dependency array Co-Authored-By: Claude <noreply@anthropic.com>

When e.preventDefault() stops Radix's default focus behaviour the dialog has no focused element, which breaks keyboard navigation and screen readers. Explicitly focus the dialog content after preventing the default in all three sites: taxa-list-details, species, and occurrences. Co-Authored-By: Claude <noreply@anthropic.com>

IsActiveStaffOrReadOnly blocks any non-staff user from POST/DELETE, but the intent of taxa lists is that any project member can manage them. Added IsProjectMemberOrReadOnly to ami/base/permissions.py: safe methods are open; unsafe methods check project.members via the active project resolved by ProjectMixin.get_active_project(). Reverted test users back to plain create_user — Project.objects.create with owner= auto-adds the owner as a member via ensure_owner_membership, which is all the permission now requires. Co-Authored-By: Claude <noreply@anthropic.com>

netlify · 2026-02-05T07:24:02Z

✅ Deploy Preview for antenna-preview ready!

Name	Link
🔨 Latest commit	`4b2d1c7`
🔍 Latest deploy log	https://app.netlify.com/projects/antenna-preview/deploys/6986107269a12300087d907d
😎 Deploy Preview	https://deploy-preview-1119--antenna-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 61 (🔴 down 5 from production) Accessibility: 80 (no change from production) Best Practices: 100 (no change from production) SEO: 92 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify project configuration.

coderabbitai · 2026-02-05T07:24:07Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

🔍 Trigger a full review

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix/taxa-list-review-feedback

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

- get_taxa_list() now validates that the taxa list belongs to the active project, preventing cross-project manipulation via URL params - Added setDetailBreadcrumb to useEffect dependency array Co-Authored-By: Claude <noreply@anthropic.com>

annavik · 2026-02-06T15:42:06Z

ui/src/data-services/hooks/entities/utils.ts

    ...(description ? { description } : {}),
    ...(name ? { name } : {}),
-    project_id: projectId,
+    project: projectId,


This change seems risky, I will revert

annavik · 2026-02-06T15:43:38Z

ui/src/data-services/models/entity.ts


  get createdAt(): string | undefined {
-    if (!this._data.updated_at) {
+    if (!this._data.created_at) {


annavik · 2026-02-06T15:49:35Z

ui/src/pages/occurrences/occurrences.tsx

        onOpenAutoFocus={(e) => {
          /* Prevent tooltip auto focus */
          e.preventDefault()
+          ;(e.currentTarget as HTMLElement).focus()


If we want to enable auto focus again, the correct way would be to skip onOpenAutoFocus. The code in this method will now first disable, then enable again, so a bit unnecessary.

I disabled it because I was annoyed to see the rank tooltip show up on focus :) However it's better for accessibility to have it, so maybe not a good idea.

I'll push an update here.

annavik · 2026-02-06T15:50:43Z

ui/src/pages/taxa-list-details/remove-taxa-list-taxon/remove-taxa-list-taxon-dialog.tsx

        <FormSection
          title={translate(STRING.REMOVE_TAXA_LIST_TAXON)}
          description={translate(
-            STRING.MESSAGE_MESSAGE_REMOVE_TAXA_LIST_TAXON_CONFIRM


annavik · 2026-02-06T15:53:05Z

ui/src/pages/taxa-lists/taxa-lists.tsx

        title={translate(STRING.NAV_ITEM_TAXA_LISTS)}
        subTitle={translate(STRING.RESULTS, {
-          total: taxaLists?.length ?? 0,
+          total: total ?? taxaLists?.length ?? 0,


This can actually be simplified further now when we have pagination support!

annavik · 2026-02-06T16:07:17Z

Thanks for the fixes @mihow !! I pushed some tweaks and also tested things out again here. One thing I noticed in this branch is that if I'm a superuser, but not a member of the project, I cannot add or remove taxa from lists. This is not the case in the branch feat/taxa-lists.

mihow · 2026-02-06T19:16:41Z

Thanks for the fixes @mihow !! I pushed some tweaks and also tested things out again here. One thing I noticed in this branch is that if I'm a superuser, but not a member of the project, I cannot add or remove taxa from lists. This is not the case in the branch feat/taxa-lists.

Interesting discovery! I will look over the permissions for taxa lists as a whole again. I like the error handling you have now! "You do not have permission to perform this action" in the bar above the Add Taxon form! That is slick.

I also opened this ticket to look more into permissions for all data types that can be assigned to multiple projects.
#1120

mihow and others added 7 commits February 4, 2026 22:45

mihow mentioned this pull request Feb 5, 2026

Configurable taxa lists #1094

Open

mihow and others added 2 commits February 5, 2026 09:55

Merge branch 'feat/taxa-lists' into fix/taxa-list-review-feedback

261cb2e

annavik reviewed Feb 6, 2026

View reviewed changes

fix: cleanup UI fixes

4b2d1c7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: address taxa-list PR review feedback#1119

fix: address taxa-list PR review feedback#1119
mihow wants to merge 10 commits intofeat/taxa-listsfrom
fix/taxa-list-review-feedback

mihow commented Feb 5, 2026

Uh oh!

netlify bot commented Feb 5, 2026 •

edited

Loading

Uh oh!

coderabbitai bot commented Feb 5, 2026

Review skipped

Uh oh!

annavik Feb 6, 2026

Uh oh!

annavik Feb 6, 2026

Uh oh!

annavik Feb 6, 2026

Uh oh!

annavik Feb 6, 2026

Uh oh!

annavik Feb 6, 2026

Uh oh!

annavik commented Feb 6, 2026

Uh oh!

mihow commented Feb 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mihow commented Feb 5, 2026

Summary

Test plan

Uh oh!

netlify bot commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for antenna-preview ready!

Uh oh!

coderabbitai bot commented Feb 5, 2026

Review skipped

Uh oh!

annavik Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

annavik Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

annavik Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

annavik Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

annavik Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

annavik commented Feb 6, 2026

Uh oh!

mihow commented Feb 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

netlify bot commented Feb 5, 2026 •

edited

Loading