fix: handle null occurrence determination crashes in UI#1186
Closed
fix: handle null occurrence determination crashes in UI#1186
Conversation
Occurrences can have a NULL determination FK when pipeline result saving crashes before completion. The frontend Occurrence model and all consuming components now guard against this with null checks, optional chaining, and fallback display text using STRING.UNKNOWN. Co-Authored-By: Claude <noreply@anthropic.com>
✅ 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. |
Contributor
📝 WalkthroughWalkthroughThis PR introduces data integrity reconciliation utilities to detect and fix occurrences with missing determinations despite having classifications. It adds a backend module with reconciliation logic, integrates it into the ML pipeline's result-saving process, and wraps it in a Django management command and Celery task. Frontend updates make determination taxons optional throughout the UI with defensive null checks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
mihow
force-pushed
the
fix/null-determination-crash
branch
from
26b8520 to
8453f7f
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes UI crashes and backend data issues caused by occurrences with determination: null, ensuring the UI renders safely and adding server-side reconciliation to repair affected rows.
Changes:
- Hardened occurrence UI rendering and actions to tolerate missing determination/taxon (optional chaining + guarded quick actions).
- Updated UI occurrence models to treat determination/taxon as optional and avoid unsafe access.
- Added backend integrity reconciliation (callable via pipeline post-save hook, Celery task, and a management command).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/pages/occurrences/occurrence-gallery.tsx | Avoids null dereferences when rendering taxon name/actions in gallery cards. |
| ui/src/pages/occurrences/occurrence-columns.tsx | Renders “Unknown” when taxon missing; hides quick actions when no taxon. |
| ui/src/pages/occurrences/occurrence-actions.tsx | Filters null taxa for bulk quick actions and agree payload generation. |
| ui/src/pages/occurrence-details/occurrence-details.tsx | Guards header taxon rendering and action buttons when no taxon. |
| ui/src/pages/occurrence-details/identification-card/machine-prediction.tsx | Prevents null dereference when checking “applied” prediction. |
| ui/src/data-services/models/occurrence.ts | Makes determination/taxon optional and adds null guards throughout getters/logic. |
| ui/src/data-services/models/occurrence-details.ts | Guards applied/overridden computation when no determination taxon. |
| ami/ml/models/pipeline.py | Adds post-save reconciliation hook for missing determinations. |
| ami/main/tasks.py | Adds periodic Celery task to run integrity reconciliation. |
| ami/main/management/commands/check_data_integrity.py | Adds CLI command to audit/fix missing determinations with dry-run support. |
| ami/main/integrity.py | Implements detection + reconciliation logic for missing determinations. |
Comments suppressed due to low confidence (1)
ui/src/pages/occurrences/occurrence-actions.tsx:87
- In the bulk actions,
Agreenow filters out occurrences withdeterminationTaxonwhen buildingagreeParams, butoccurrenceIds(passed touseCreateIdentifications) andallAgreedare still computed over all selected occurrences. This can lead to misleading UI states (e.g., the request succeeds and the button shows “Confirmed” viaisSuccesseven though some selected occurrences had no determination to confirm, oragreeParamsmay be empty). Consider derivingoccurrenceIds/allAgreedfrom the same filtered set (or disabling/hiding the confirm action when any selected occurrence lacks a determination).
const occurrenceIds = occurrences.map((occurrence) => occurrence.id)
const agreeParams: IdentificationFieldValues[] = useMemo(
() =>
occurrences
.filter((occurrences) => {
const agreed = userInfo?.id
? userInfo.id === occurrences.determinationVerifiedBy?.id
: false
return !agreed
})
.filter((occurrence) => !!occurrence.determinationTaxon)
.map((occurrence) => ({
agreeWith: {
identificationId: occurrence.determinationIdentificationId,
predictionId: occurrence.determinationPredictionId,
},
occurrenceId: occurrence.id,
taxonId: occurrence.determinationTaxon!.id,
})),
[occurrences]
)
const { createIdentifications, isLoading, isSuccess, error } =
useCreateIdentifications(occurrenceIds)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 1
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/occurrences/occurrence-actions.tsx (1)
63-83:⚠️ Potential issue | 🟡 MinorMissing dependency in
useMemo.The
agreeParamscomputation referencesuserInfo?.idon Line 68 but the dependency array only includes[occurrences]. IfuserInfochanges, the memoized value won't recompute.🛠️ Proposed fix
[occurrences] + [occurrences, userInfo]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/occurrences/occurrence-actions.tsx` around lines 63 - 83, The memoized agreeParams computed in the agreeParams useMemo references userInfo?.id but its dependency array only includes [occurrences], so changes to userInfo won't trigger recomputation; update the dependency array for the useMemo that defines agreeParams to include userInfo (or userInfo?.id) in addition to occurrences so the value is recalculated when the current user changes (keep the rest of the logic in agreeParams unchanged).
🧹 Nitpick comments (4)
ami/main/integrity.py (4)
78-82: Same redundantselect_related("determination")here.else: occurrences = get_occurrences_missing_determination( project_id=project_id, job_id=job_id, - ).select_related("determination") + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/main/integrity.py` around lines 78 - 82, The call to select_related("determination") is redundant when assigning occurrences from get_occurrences_missing_determination; remove the .select_related("determination") suffix so occurrences is assigned directly from get_occurrences_missing_determination(project_id=project_id, job_id=job_id). This change affects the occurrences assignment in the else branch where get_occurrences_missing_determination is used.
69-77:select_related("determination")is redundant for null-determination filter.The queryset filters for
determination__isnull=True, so the relateddeterminationobject will always beNone. Theselect_related("determination")call adds no value here.Suggested fix
occurrences = ( Occurrence.objects.filter( pk__in=occurrence_ids, determination__isnull=True, detections__classifications__isnull=False, ) .distinct() - .select_related("determination") )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/main/integrity.py` around lines 69 - 77, In the occurrences queryset (Occurrence.objects.filter(...)) you call select_related("determination") even though the filter uses determination__isnull=True, so the related determination will always be None; remove the .select_related("determination") call from the occurrences assignment in ami/main/integrity.py (the Occurrence.objects.filter(...) -> occurrences definition) to avoid the redundant join.
41-49: Silent fallthrough when job has no pipeline may cause unexpected behavior.When
job_idis provided butjob.pipelineisNone, the function silently ignores thejob_idfilter and only appliesproject_id=job.project_id. This could reconcile more occurrences than intended. Consider logging a warning or raising an error when the job has no associated pipeline.Suggested improvement
if job_id is not None: from ami.jobs.models import Job job = Job.objects.get(pk=job_id) if job.pipeline: qs = qs.filter( detections__classifications__algorithm__in=job.pipeline.algorithms.all(), project_id=job.project_id, ) + else: + logger.warning(f"Job {job_id} has no pipeline, filtering by project_id only") + qs = qs.filter(project_id=job.project_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/main/integrity.py` around lines 41 - 49, The code currently ignores job_id when job.pipeline is None, causing a silent fallthrough; change the block that loads Job (Job.objects.get(pk=job_id)) to explicitly handle a missing pipeline by either raising an error or logging and aborting: check if job.pipeline is None and then raise a ValueError (e.g. ValueError(f"Job {job_id} has no pipeline")) or call logger.warning(...) and return qs.none(); do this in the same scope where job and qs are used (referencing job_id, Job, job.pipeline, and qs) and add any needed import for the logger or exception handling.
84-91: Minor TOCTOU:count()anditerator()execute separate queries.The queryset is evaluated twice: once for
count()(line 84) and again foriterator()(line 91). If rows are inserted/deleted between these calls,result.checkedwon't match the actual iteration count. This is unlikely to cause issues in practice since reconciliation is idempotent, but for accuracy you could iterate first and count as you go.Alternative approach
- result = ReconcileResult(checked=occurrences.count()) - - if result.checked == 0 or dry_run: + if dry_run: + result = ReconcileResult(checked=occurrences.count()) return result - logger.info(f"Found {result.checked} occurrences missing determination") + result = ReconcileResult() for occurrence in occurrences.iterator(): + result.checked += 1 try: updated = update_occurrence_determination(occurrence, current_determination=None, save=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/main/integrity.py` around lines 84 - 91, Avoid the separate count() call to prevent the TOCTOU mismatch: do not call occurrences.count(); instead, if dry_run is true return early, otherwise iterate occurrences with occurrences.iterator(), increment a local counter for each iteration, set result.checked to that counter after the loop (or update result.checked as you go), and then call logger.info using result.checked; reference ReconcileResult, occurrences.iterator(), result.checked, dry_run and logger.info to locate and change the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ami/main/tasks.py`:
- Around line 8-21: The periodic task check_data_integrity currently calls
reconcile_missing_determinations() without bounds which can exceed the celery
soft_time_limit, overload the DB, and allow overlapping runs; modify
check_data_integrity to acquire a distributed/concurrency lock (e.g., via Django
cache lock or a DB advisory lock) at start and release it at end, and call
reconcile_missing_determinations with batching parameters (e.g.,
batch_size/limit and an iterator or cursor) or an optional max_runtime to
process limited rows per invocation and return aggregated partial counts so
repeated scheduled runs can safely finish the full scan without overlap.
---
Outside diff comments:
In `@ui/src/pages/occurrences/occurrence-actions.tsx`:
- Around line 63-83: The memoized agreeParams computed in the agreeParams
useMemo references userInfo?.id but its dependency array only includes
[occurrences], so changes to userInfo won't trigger recomputation; update the
dependency array for the useMemo that defines agreeParams to include userInfo
(or userInfo?.id) in addition to occurrences so the value is recalculated when
the current user changes (keep the rest of the logic in agreeParams unchanged).
---
Nitpick comments:
In `@ami/main/integrity.py`:
- Around line 78-82: The call to select_related("determination") is redundant
when assigning occurrences from get_occurrences_missing_determination; remove
the .select_related("determination") suffix so occurrences is assigned directly
from get_occurrences_missing_determination(project_id=project_id,
job_id=job_id). This change affects the occurrences assignment in the else
branch where get_occurrences_missing_determination is used.
- Around line 69-77: In the occurrences queryset
(Occurrence.objects.filter(...)) you call select_related("determination") even
though the filter uses determination__isnull=True, so the related determination
will always be None; remove the .select_related("determination") call from the
occurrences assignment in ami/main/integrity.py (the
Occurrence.objects.filter(...) -> occurrences definition) to avoid the redundant
join.
- Around line 41-49: The code currently ignores job_id when job.pipeline is
None, causing a silent fallthrough; change the block that loads Job
(Job.objects.get(pk=job_id)) to explicitly handle a missing pipeline by either
raising an error or logging and aborting: check if job.pipeline is None and then
raise a ValueError (e.g. ValueError(f"Job {job_id} has no pipeline")) or call
logger.warning(...) and return qs.none(); do this in the same scope where job
and qs are used (referencing job_id, Job, job.pipeline, and qs) and add any
needed import for the logger or exception handling.
- Around line 84-91: Avoid the separate count() call to prevent the TOCTOU
mismatch: do not call occurrences.count(); instead, if dry_run is true return
early, otherwise iterate occurrences with occurrences.iterator(), increment a
local counter for each iteration, set result.checked to that counter after the
loop (or update result.checked as you go), and then call logger.info using
result.checked; reference ReconcileResult, occurrences.iterator(),
result.checked, dry_run and logger.info to locate and change the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93542514-5126-40e1-9831-971a2a5e7b74
📒 Files selected for processing (11)
ami/main/integrity.pyami/main/management/commands/check_data_integrity.pyami/main/tasks.pyami/ml/models/pipeline.pyui/src/data-services/models/occurrence-details.tsui/src/data-services/models/occurrence.tsui/src/pages/occurrence-details/identification-card/machine-prediction.tsxui/src/pages/occurrence-details/occurrence-details.tsxui/src/pages/occurrences/occurrence-actions.tsxui/src/pages/occurrences/occurrence-columns.tsxui/src/pages/occurrences/occurrence-gallery.tsx
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
|
This has been merged into the demo deployment and tested there |
Collaborator
Author
|
@copilot instead of this fix, let's filter out occurrences without determinations from the occurrences list responses - currently they are filtered if apply default filters is true, but they should always be filtered. If we don't already have a QuerySet method on Occurrences, consider adding one called |
Collaborator
Author
|
Will filter more aggressively on backend and add a periodic data integrity check #1188 |
Contributor
Updated def valid(self):
return self.exclude(detections__isnull=True).exclude(determination__isnull=True)Since Also added
However, I'm unable to push these changes because PR #1186 is closed and the branch is now protected. The changes are committed locally on both |
Copilot stopped work on behalf of
mihow due to an error
March 26, 2026 00:48
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.
OccurrenceQuerySet.valid()to also exclude occurrences withdetermination=nullTestOccurrencesWithoutDeterminationtests to verify behavior🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.