Fix 2-digit year timestamp parsing, sync error reporting, and captures columns#1159
Fix 2-digit year timestamp parsing, sync error reporting, and captures columns#1159
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
⌛ 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. 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds 2‑digit year timestamp parsing, surfaces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ui/src/pages/captures/capture-columns.tsx (1)
137-148: Both columns sort bypath— intentional?The
filenamecolumn usessortField: 'path', meaning sorting by filename will actually sort by the full path. This is a reasonable choice (maintains directory grouping), but worth confirming this is the intended UX rather than sorting by the filename segment alone.If filename-only sorting is desired, a dedicated backend sort field would be needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/captures/capture-columns.tsx` around lines 137 - 148, The filename column currently uses sortField: 'path' (in the column definition with id: 'filename' and renderCell: (item: Capture) => <BasicTableCell value={item.filename} />), which sorts by the full path; if you want filename-only sorting change sortField to 'filename' (and ensure the backend supports a filename sort field), otherwise explicitly document the intended behavior (keep sortField: 'path') so it's clear this choice groups by directory; if the backend lacks a filename sort field, add one server-side before switching the sortField.
🤖 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/models.py`:
- Around line 810-816: The except block around _create_source_image_for_sync
currently only logs errors via job.logger if job exists, so when job is None
exceptions are swallowed and tracebacks lost; update the handler to always
record the error (use the module-level logger or logging.getLogger(__name__))
and include the full traceback (e.g., logger.exception or include
traceback.format_exc()) while still incrementing failed and continuing; keep the
job.logger call when job exists but also emit the same detailed log
unconditionally for visibility in non-job runs.
---
Nitpick comments:
In `@ui/src/pages/captures/capture-columns.tsx`:
- Around line 137-148: The filename column currently uses sortField: 'path' (in
the column definition with id: 'filename' and renderCell: (item: Capture) =>
<BasicTableCell value={item.filename} />), which sorts by the full path; if you
want filename-only sorting change sortField to 'filename' (and ensure the
backend supports a filename sort field), otherwise explicitly document the
intended behavior (keep sortField: 'path') so it's clear this choice groups by
directory; if the backend lacks a filename sort field, add one server-side
before switching the sortField.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
ami/jobs/models.pyami/main/api/serializers.pyami/main/models.pyami/utils/dates.pyami/utils/tests.pyui/src/data-services/models/capture.tsui/src/pages/captures/capture-columns.tsxui/src/pages/captures/captures.tsxui/src/utils/language.ts
There was a problem hiding this comment.
Pull request overview
This PR improves capture ingestion and UI display for edge-case timestamps, adds per-image failure reporting to data storage sync jobs, and exposes capture path/filename in the captures table.
Changes:
- Extend backend filename timestamp parsing to support 12-digit
YYMMDDHHMMSSpatterns and add unit tests. - Add per-file exception handling and a “Failed” counter to data storage sync job progress updates.
- Update UI capture model to avoid displaying epoch time for null timestamps, and add hidden-by-default Filename/Path table columns (with
pathadded to the capture list serializer).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
ami/utils/dates.py |
Add regex + %y parsing support for 2-digit year timestamps in filenames. |
ami/utils/tests.py |
Add unit tests covering 2-digit year filename parsing. |
ami/main/models.py |
Track/log sync failures per object and report them via job progress. |
ami/jobs/models.py |
Add “Failed” stage param for the data storage sync job. |
ami/main/api/serializers.py |
Expose path in the capture list serializer. |
ui/src/data-services/models/capture.ts |
Return blank labels for null timestamps; add path/filename getters; make date optional. |
ui/src/pages/captures/capture-columns.tsx |
Add Filename and Path columns (hidden by default), both sortable by path. |
ui/src/pages/captures/captures.tsx |
Add filename and path to persisted column visibility defaults (off). |
ui/src/utils/language.ts |
Add FIELD_LABEL_FILENAME string constant and English label. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ui/src/pages/session-details/playback/capture-navigation/capture-navigation.tsx (1)
61-68: Consider adding the same explanatory comment here for consistency.The
goToNextfunction uses the same non-null assertion pattern asgoToPrevbut lacks the explanatory comment. While the invariant is documented once, duplicating it here would improve readability for anyone reading this function in isolation.📝 Suggested comment addition
const goToNext = () => { if (!activeCapture) { return } + // Session captures always have dates (NULL timestamps excluded from events) const nextCaptureId = snapToDetections ? findClosestCaptureId({ minDate: activeCapture.date!,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/session-details/playback/capture-navigation/capture-navigation.tsx` around lines 61 - 68, Duplicate the explanatory non-null invariant comment used in goToPrev next to the non-null assertion in goToNext: add the same comment above the nextCaptureId expression (which uses activeCapture.date! and snapToDetections) to document why the non-null assertion is safe; reference the goToPrev comment as the source text and place the duplicate immediately before the nextCaptureId assignment in the capture-navigation.tsx file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@ui/src/pages/session-details/playback/capture-navigation/capture-navigation.tsx`:
- Around line 61-68: Duplicate the explanatory non-null invariant comment used
in goToPrev next to the non-null assertion in goToNext: add the same comment
above the nextCaptureId expression (which uses activeCapture.date! and
snapToDetections) to document why the non-null assertion is safe; reference the
goToPrev comment as the source text and place the duplicate immediately before
the nextCaptureId assignment in the capture-navigation.tsx file.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ami/jobs/models.pyami/main/api/views.pyami/main/models.pyui/src/pages/session-details/playback/capture-navigation/capture-navigation.tsxui/src/pages/session-details/playback/session-captures-slider/session-captures-slider.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- ami/main/models.py
- ami/jobs/models.py
|
I'm also unsure why we would ever want to import images with NULL timestamps. That will require a data migration, but seems like a good integrity change. I went ahead and stopped importing images if the timestamp cannot be parsed. |
mihow
force-pushed
the
fix/2digit-year-timestamps-and-sync-improvements
branch
from
bfb29e2 to
6351030
Compare
Some cameras (e.g. Farmscape/NSCF) produce filenames with 12-digit timestamps using a 2-digit year. These were silently returning NULL, causing images to show epoch dates in the UI and preventing session grouping. Add a new regex pattern for 12-digit timestamps and use %y format for parsing. Co-Authored-By: Claude <noreply@anthropic.com>
Images with unparseable timestamps can't be grouped into events (group_images_into_events already excluded them), so importing them just created orphaned records. Now they are skipped at sync time. Also wraps per-image processing in try/except, tracks a failed counter, and reports it as a "Failed" stage param in the job progress UI. Warns on suspicious pre-2000 timestamps. Co-Authored-By: Claude <noreply@anthropic.com>
Add two new columns to the captures table (hidden by default, toggled via column settings). Expose the path field in the list serializer and add it to sortable fields. Derive filename from path on the frontend. Co-Authored-By: Claude <noreply@anthropic.com>
mihow
force-pushed
the
fix/2digit-year-timestamps-and-sync-improvements
branch
from
6351030 to
1a25c84
Compare
Summary
2-digit year timestamp parsing: Some cameras produce filenames with YYMMDDHHMMSS (12-digit) timestamps instead of the expected YYYYMMDDHHMMSS (14-digit). These were silently returning NULL, causing images to show "Dec 31, 1969" in the UI and preventing session grouping. Added a new regex pattern and
%yformat handling.Skip null-timestamp images and report failures: Images with unparseable timestamps are now skipped during sync rather than imported with NULL. They can't be grouped into events anyway (
group_images_into_eventsalready excluded them), so importing them just created orphaned records. Per-image processing is wrapped in try/except, afailedcounter is tracked and reported as a "Failed" stage param in the job progress UI. Warns on suspicious pre-2000 timestamps. If timestamp parsing is improved later, a re-sync will pick up previously skipped images.Filename/path columns: Added two new columns to the captures table (hidden by default, toggled via column settings). Exposes the
pathfield in the list serializer and derivesfilenamefrom it on the frontend.Related
Screenshots
Before: Null timestamps showing epoch date
After: Correct timestamps parsed from 2-digit year filenames
Filename and Path columns (toggled on via column settings)
Sync job error reporting (Failed stage param in job progress)
Test plan