Sync station storage sources from the stations list, per station or all at once#1357
Sync station storage sources from the stations list, per station or all at once#1357mihow wants to merge 11 commits into
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
Adds a floating action button to every row in the Stations (deployments)
list table, beside the existing Delete button. Clicking it opens a
confirmation dialog; confirming triggers the asynchronous data-storage
sync job that scans the station's connected storage source and imports
any new captures.
Reuses the existing POST /deployments/{id}/sync/ endpoint and the
useSyncDeploymentSourceImages hook (already powering the detail-view
'Sync now' button). On success the dialog shows a link to the created
job; a station with no data source returns 400, surfaced in the dialog.
The button is shown only to users with update permission on the station.
Co-Authored-By: Claude <noreply@anthropic.com>
- Reset the sync mutation state when the dialog opens. The hook is mounted
for the whole table row, so success/error/data otherwise survived a close
and left the Sync button permanently disabled after one sync. The hook now
exposes reset(); the dialog calls it on open for a fresh attempt each time.
- Swallow the promise rejection at the call site. The no-data-source 400 is
an intended path surfaced via FormError, so the bare mutateAsync call would
otherwise log an unhandled rejection on every such click.
- Use a clearer dialog title ('Sync captures') instead of the bare 'Sync'.
Co-Authored-By: Claude <noreply@anthropic.com>
✅ Deploy Preview for antenna-preview canceled.
|
✅ Deploy Preview for antenna-ssec canceled.
|
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds a bulk "sync-all" deployment endpoint alongside the existing per-deployment sync action, a ChangesSync Feature Implementation
Estimated code review effort: 3 (Moderate) | ~30 minutes Planning Documentation
Sequence Diagram(s)sequenceDiagram
participant User
participant DeploymentsPage
participant SyncAllDeploymentsDialog
participant useSyncAllDeployments
participant DeploymentViewSet
participant DataStorageSyncJob
User->>DeploymentsPage: view deployments list
DeploymentsPage->>SyncAllDeploymentsDialog: render if syncableCount > 0
User->>SyncAllDeploymentsDialog: confirm sync all
SyncAllDeploymentsDialog->>useSyncAllDeployments: syncAllDeployments(projectId)
useSyncAllDeployments->>DeploymentViewSet: POST /deployments/sync-all/
DeploymentViewSet->>DataStorageSyncJob: enqueue job per connected deployment
DeploymentViewSet-->>useSyncAllDeployments: { job_ids, queued, project_id }
useSyncAllDeployments-->>SyncAllDeploymentsDialog: success data
SyncAllDeploymentsDialog-->>User: show job link
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
…b consolidation) [skip ci] Co-Authored-By: Claude <noreply@anthropic.com>
The sync endpoint (and any view that does `raise ValidationError("message")`)
returns the reason as a top-level JSON list of strings, for example
["Deployment must have a data source to sync captures from"]. parseServerError
only handled object-shaped error bodies, so that shape fell through to the
generic axios "Request failed with status code 400" and the real reason was
lost. Handle the top-level list explicitly and join its strings into the
message, and add a test for the shape.
Co-Authored-By: Claude <noreply@anthropic.com>
FormError's `inDialog` variant is position: sticky at top: 72px, which assumes a 72px Dialog header. The sync dialog is compact and keeps its title inside the FormSection, so the sticky error was pushed down and overlapped the title. Drop `inDialog` to match DeleteForm (the other compact confirmation dialog), so the error banner stacks cleanly above the title. Co-Authored-By: Claude <noreply@anthropic.com>
…d stations Backend: - Expose data_source_connected on the stations list serializer (a read-only boolean from the data_source foreign key id, so it adds no query). - Add DeploymentViewSet.sync_all (POST /deployments/sync-all/?project_id=) that enqueues one DataStorageSyncJob per connected station in the project — separate jobs, matching the per-row sync action and the admin bulk action. Since ObjectPermission only guards detail actions via get_object(), this detail=False action checks the sync permission itself with the codebase's transient-probe pattern. Adds a permission-matrix and field/enqueue test. Frontend: - The per-row Sync button shows only when the station has a storage source, so it no longer appears where a sync would fail with a 400. - A "Sync all" button in the Stations list header syncs every connected station the user can update; the dialog reports how many jobs will run and links to the Jobs tab on success. Co-Authored-By: Claude <noreply@anthropic.com>
MLDataManager already had run_data_storage_sync_job (it could run/retry a sync job) but not sync_deployment (which gates starting a sync from a station), so it could manage a sync job without being able to initiate one. Grant sync_deployment to MLDataManager so ML data managers can trigger syncs, matching ProjectManager. - roles.py: add SYNC_DEPLOYMENT to MLDataManager (ProjectManager inherits it). - Data migration backfills the permission onto existing projects' MLDataManager role groups; new projects get it via create_roles_for_project. - Frontend: gate the per-row and Sync all buttons on (canUpdate || canSync) so ML data managers see them. The canUpdate arm keeps them visible for superusers, who do not receive the sync permission in user_permissions. - Extend the permission-matrix test: MLDataManager 200; Researcher, Identifier, BasicMember, and non-members 403. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds “Sync” actions to the Stations (deployments) list so operators can enqueue storage-source capture sync jobs per station or for all connected stations, while extending permissions so ML data managers can initiate syncs and improving server-error parsing to surface DRF plain-string validation messages.
Changes:
- UI: Adds per-row Sync and header “Sync all” dialogs, gated by permissions and whether a station has a connected storage source.
- API: Adds
data_source_connectedto the deployments list serializer and introduces a bulkPOST /deployments/sync-all/?project_id=endpoint that enqueues one sync job per connected station. - Permissions: Grants
sync_deploymentto MLDataManager (plus a data migration for existing projects) and improves user-facing error messages for sync failures.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ui/src/utils/user/types.ts | Adds a sync user-permission enum value for deployment syncing. |
| ui/src/utils/parseServerError/parseServerError.ts | Extends error parsing to support DRF top-level list validation errors. |
| ui/src/utils/parseServerError/parseServerError.test.ts | Adds coverage for the new top-level-list error format. |
| ui/src/utils/language.ts | Adds translatable strings for Sync/Sync all dialogs and job links. |
| ui/src/pages/deployments/sync-deployment-dialog.tsx | New per-row confirmation dialog that triggers /deployments/{id}/sync/ and links to the created job. |
| ui/src/pages/deployments/sync-all-deployments-dialog.tsx | New bulk confirmation dialog that triggers /deployments/sync-all/ and links to Jobs. |
| ui/src/pages/deployments/deployments.tsx | Adds “Sync all” button in the list header when at least one station is syncable. |
| ui/src/pages/deployments/deployment-columns.tsx | Adds per-row Sync action to the stations list toolbar when eligible. |
| ui/src/data-services/models/deployment.ts | Surfaces canSync and dataSourceConnected derived properties for UI gating. |
| ui/src/data-services/hooks/deployments/useSyncDeploymentSourceImages.ts | Exposes mutation reset() so dialogs reopen with fresh state. |
| ui/src/data-services/hooks/deployments/useSyncAllDeployments.ts | New hook for the bulk sync-all endpoint with cache invalidation. |
| docs/claude/planning/2026-06-29-station-sync-action-NEXT.md | Planning/handoff notes for the feature work. |
| docs/claude/planning/2026-06-29-station-sync-action-design.md | Design doc capturing UX decisions and backend follow-ups. |
| ami/users/roles.py | Grants SYNC_DEPLOYMENT to the MLDataManager role. |
| ami/main/tests.py | Adds API tests covering data_source_connected and the sync-all endpoint behavior/permissions. |
| ami/main/migrations/0095_grant_sync_deployment_to_mldatamanager.py | Data migration intended to backfill sync permissions for existing MLDataManager role groups. |
| ami/main/api/views.py | Refactors per-deployment sync job enqueueing + adds sync_all bulk action. |
| ami/main/api/serializers.py | Adds data_source_connected to the deployments list serializer. |
Comments suppressed due to low confidence (1)
ui/src/utils/parseServerError/parseServerError.ts:22
- For
non_field_errors/detail, DRF commonly returns an array of strings (e.g.{ non_field_errors: ["..."] }). The current code assignsdetailsdirectly tomessage, which can makemessagean array at runtime and break callers that expect a string.
Object.entries(data).forEach(([key, details]) => {
if (key && details) {
if (key === 'non_field_errors' || key === 'detail') {
message = details as string
} else {
fieldErrors.push({ key, message: `${(details as string[])[0]}` })
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
ui/src/pages/deployments/sync-all-deployments-dialog.tsx (1)
27-36: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSame reopen-resets-loading race as the per-station sync dialog, with a larger blast radius.
Closing this dialog mid-request and reopening it calls
reset(), clearingisLoading/isSuccessbefore the original bulk request resolves, which re-enables "Sync all" and could enqueue a second batch of sync jobs for every connected station in the project. See the same concern raised inui/src/pages/deployments/sync-deployment-dialog.tsx— the fix should likely be applied consistently to both dialogs.Also applies to: 51-59
🤖 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/deployments/sync-all-deployments-dialog.tsx` around lines 27 - 36, The sync-all dialog has the same reopen-reset race as the per-station sync dialog: resetting the hook state in Dialog.Root on reopen clears loading/success while the original bulk request may still be in flight. Update SyncAllDeploymentsDialog to avoid calling reset() in onOpenChange when reopening during an active request, and apply the same pattern used in SyncDeploymentDialog so "Sync all" stays disabled until the current batch finishes.
🧹 Nitpick comments (2)
ui/src/data-services/hooks/deployments/useSyncAllDeployments.ts (1)
1-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting shared sync-mutation logic.
This hook is structurally near-identical to
useSyncDeploymentSourceImages(same query invalidation set, same shape). A small shared factory could reduce duplication if more sync-style mutations get added later.🤖 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/data-services/hooks/deployments/useSyncAllDeployments.ts` around lines 1 - 39, The sync-all deployments hook duplicates the same mutation/invalidation pattern used by the similar sync hook, so extract the shared mutation setup into a reusable factory or helper. Reuse the common logic from useSyncAllDeployments and useSyncDeploymentSourceImages for the axios mutation, query invalidations, and returned fields, while keeping only the endpoint-specific pieces (like the URL and payload) in each hook. Make the shared abstraction easy to extend for future sync-style mutations.ui/src/pages/deployments/deployment-columns.tsx (1)
235-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate eligibility predicate for sync UI.
The
(item.canUpdate || item.canSync) && item.dataSourceConnectedcheck here is duplicated verbatim inui/src/pages/deployments/deployments.tsx(syncableCount filter). Consider exposing a singlecanSyncNow-style getter on theDeploymentmodel so both call sites stay in sync if eligibility rules change.🤖 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/deployments/deployment-columns.tsx` around lines 235 - 237, The sync eligibility check is duplicated between the deployment columns UI and the deployments list filter, so update the Deployment model to expose a single canSyncNow-style getter or equivalent helper and use it from both places. Refactor the conditional around SyncDeploymentDialog in deployment-columns.tsx and the syncableCount filter in deployments.tsx to call the shared predicate, keeping the eligibility rules centralized on Deployment.
🤖 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/migrations/0095_grant_sync_deployment_to_mldatamanager.py`:
- Around line 23-39: The migration only grants the model-level sync permission
to *_MLDataManager groups, so existing users still fail
Deployment.check_permission(..., "sync") because
user.has_perm("sync_deployment", project) needs an object-level grant too.
Update grant_sync_to_mldatamanager to mirror create_roles_for_project by also
creating the project-scoped GroupObjectPermission entry for each matching Group,
using the same Permission and project ContentType lookup; keep the existing
group.permissions.add(perm) and ensure revoke_sync_from_mldatamanager remains
the inverse.
In `@docs/claude/planning/2026-06-29-station-sync-action-design.md`:
- Line 4: The scope statement in the planning doc conflicts with the addendum
because it says “Frontend only” while later sections describe backend
serializer, bulk endpoint, and permission work. Update the scope line in the
document to clearly mark it as the initial phase only, or rephrase it as
historical context so it does not contradict the later backend-related sections.
In `@docs/claude/planning/2026-06-29-station-sync-action-NEXT.md`:
- Around line 11-15: Update the scope note in the planning doc to match the
final PR instead of claiming “Frontend-only, no backend change, no migration.”
Refer to the deployment sync feature description and revise it to reflect that
the shipped work also includes backend serializer/permission changes and a
migration, or explicitly reframe it as phase-1 history if that is the intended
meaning.
In `@ui/src/pages/deployments/sync-deployment-dialog.tsx`:
- Around line 34-43: The dialog reopening logic in sync-deployment-dialog.tsx
clears mutation state with reset() while a sync is still in flight, which can
re-enable the Sync action too early. Update the Sync dialog flow around the
onOpenChange handler and the mutation state used by the Sync button so reopening
cannot make the action clickable during an active request; either block
close/cancel while isLoading or track a separate pending flag outside the
mutation state and use that to keep the button disabled until the request
finishes.
---
Duplicate comments:
In `@ui/src/pages/deployments/sync-all-deployments-dialog.tsx`:
- Around line 27-36: The sync-all dialog has the same reopen-reset race as the
per-station sync dialog: resetting the hook state in Dialog.Root on reopen
clears loading/success while the original bulk request may still be in flight.
Update SyncAllDeploymentsDialog to avoid calling reset() in onOpenChange when
reopening during an active request, and apply the same pattern used in
SyncDeploymentDialog so "Sync all" stays disabled until the current batch
finishes.
---
Nitpick comments:
In `@ui/src/data-services/hooks/deployments/useSyncAllDeployments.ts`:
- Around line 1-39: The sync-all deployments hook duplicates the same
mutation/invalidation pattern used by the similar sync hook, so extract the
shared mutation setup into a reusable factory or helper. Reuse the common logic
from useSyncAllDeployments and useSyncDeploymentSourceImages for the axios
mutation, query invalidations, and returned fields, while keeping only the
endpoint-specific pieces (like the URL and payload) in each hook. Make the
shared abstraction easy to extend for future sync-style mutations.
In `@ui/src/pages/deployments/deployment-columns.tsx`:
- Around line 235-237: The sync eligibility check is duplicated between the
deployment columns UI and the deployments list filter, so update the Deployment
model to expose a single canSyncNow-style getter or equivalent helper and use it
from both places. Refactor the conditional around SyncDeploymentDialog in
deployment-columns.tsx and the syncableCount filter in deployments.tsx to call
the shared predicate, keeping the eligibility rules centralized on Deployment.
🪄 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: 1c894dcd-4a2e-4865-8f8b-24f09d2e3f7d
📒 Files selected for processing (18)
ami/main/api/serializers.pyami/main/api/views.pyami/main/migrations/0095_grant_sync_deployment_to_mldatamanager.pyami/main/tests.pyami/users/roles.pydocs/claude/planning/2026-06-29-station-sync-action-NEXT.mddocs/claude/planning/2026-06-29-station-sync-action-design.mdui/src/data-services/hooks/deployments/useSyncAllDeployments.tsui/src/data-services/hooks/deployments/useSyncDeploymentSourceImages.tsui/src/data-services/models/deployment.tsui/src/pages/deployments/deployment-columns.tsxui/src/pages/deployments/deployments.tsxui/src/pages/deployments/sync-all-deployments-dialog.tsxui/src/pages/deployments/sync-deployment-dialog.tsxui/src/utils/language.tsui/src/utils/parseServerError/parseServerError.test.tsui/src/utils/parseServerError/parseServerError.tsui/src/utils/user/types.ts
…globally
The 0095 backfill added sync_deployment only to group.permissions (a global
Django permission). Authorization here is guardian object-level: get_perms(user,
project) and user.has_perm("sync_deployment", project) read the object-level
GroupObjectPermission, so the global-only grant was a silent no-op — existing
projects' MLDataManager groups would not actually gain the ability to sync.
Mirror create_roles_for_project: keep the global add for parity and, more
importantly, create the object-level GroupObjectPermission per project (reverse
removes both). Add a test that strips the object-level grant, runs the backfill,
and asserts the permission and endpoint access are restored — this fails against
the global-only version.
Reported by Copilot and CodeRabbit on the PR.
Co-Authored-By: Claude <noreply@anthropic.com>
Reopening a sync dialog reset the mutation state to offer a fresh sync, but reset() clears the loading state without cancelling the in-flight request. If the dialog was closed and reopened during a request, Sync became clickable again and could enqueue a second job for the same deployment (or a second bulk run). Guard the reset on `!isLoading`, so reopening during an active request keeps the button disabled until the request finishes. Applies to both the per-row and Sync all dialogs. Reported by CodeRabbit on the PR. Co-Authored-By: Claude <noreply@anthropic.com>
The design and handoff notes said "frontend only, no migration", which no longer matches the shipped PR (backend serializer field, sync-all endpoint, and an ML-data-manager permission with a data migration). Mark those scope lines as the initial phase and point to the addendum / final scope. Reported by CodeRabbit on the PR. Co-Authored-By: Claude <noreply@anthropic.com>
9e4f555 to
7f8e318
Compare
Summary
Operators who manage a project's stations often want to pull in newly captured
images from a station's connected storage source. Today that means opening each
station's detail page and clicking "Sync now". This brings the action to the
Stations list: a per-row "Sync" button next to Delete, and a "Sync all" button
in the header that syncs every connected station in one go. Each sync runs as the
existing background job, so nothing about how syncing works changes — only where
you can start it.
Clicking a row's Sync button opens a short confirmation dialog; confirming starts
the station's data-storage sync job and, on success, shows a link to the created
job. The Sync button only appears on stations that actually have a storage source
configured, so it is never shown where a sync would fail. "Sync all" enqueues one
job per connected station (separate jobs, one per station, matching the admin
bulk action), tells you how many will run, and links to the Jobs tab on success.
This started as a frontend-only change and grew a small backend piece: the
stations list now reports whether each station has a storage source, a new bulk
endpoint enqueues the sync jobs server-side, and ML data managers gain the
permission to sync. No schema change; a single data migration grants the sync
permission to existing projects' role groups.
Design notes and the permission analysis live in
docs/claude/planning/2026-06-29-station-sync-action-design.md.Screenshots
List of Changes
SyncDeploymentDialogin the actionsToolbarofdeployment-columns.tsx, gated onitem.canUpdate && item.dataSourceConnected. ReusesuseSyncDeploymentSourceImages(POST /deployments/{id}/sync/).SyncAllDeploymentsDialog+useSyncAllDeploymentshook (POST /deployments/sync-all/?project_id=). The header shows it only when at least one station is syncable.data_source_connectedboolean onDeploymentListSerializer(derived from thedata_sourceFK id, no extra query); surfaced asDeployment.dataSourceConnectedon the frontend.DeploymentViewSet.sync_allaction. It requiresproject_id, checks the sync permission itself (adetail=Falseaction is not covered byObjectPermission.has_object_permission), and enqueues oneDataStorageSyncJobper connected station, mirroring the admin bulk action.sync_deploymentto theMLDataManagerrole (roles.py+ a data migration backfilling existing projects' role groups). This also closes an existing gap where an ML data manager could run/retry a sync job but not start one. Identifiers and basic members still cannot sync.ValidationError(serialized as a top-level JSON list);parseServerErrornow reads that shape. Benefits every endpoint that raises a plain-string error.inDialog(sticky, header-offset) variant ofFormError, matchingDeleteForm.reset(), called on dialog open (the hooks stay mounted with the row / header).SYNC,SYNC_ALL,SYNC_CAPTURES,MESSAGE_SYNC_CONFIRM,MESSAGE_SYNC_ALL_CONFIRM,VIEW_JOB,VIEW_JOBSinui/src/utils/language.ts.Who can sync
Syncing (per-row and "Sync all") is allowed for Project managers and ML
data managers — plus superusers. Identifiers, basic members, and non-members
cannot. This is enforced by the
sync_deploymentpermission, which wasProject-manager-only; this PR grants it to ML data managers as well (they already
had
run_data_storage_sync_job, so they could run a sync job but not start one —that gap is now closed).
Notes on permissions (invisible in the diff)
canUpdate || canSync.canSyncmaps to thesyncpermission (ML data managers, Project managers). ThecanUpdatearm isthere for superusers: they receive
update/deleteinuser_permissionsbutnot
sync(guardian returns no object perms for superusers), socanUpdatekeeps the buttons visible for them.
sync_allis adetail=Falseaction, whichObjectPermissiondoes not guard(its
has_object_permissiononly runs for detail actions viaget_object()).The action therefore checks the sync permission itself against the project,
using the same transient-probe pattern the codebase already uses for other
collection actions. A permission-matrix test covers superuser / ProjectManager /
MLDataManager (200) and Researcher / Identifier / BasicMember / non-member (403).
Verification
Verified against a live docker-compose stack with Chrome DevTools (a project with
one connected and one unconnected station):
POST /deployments/{id}/sync/200 → job created → "View job" navigates.POST /deployments/sync-all/?project_id=200 → one job perconnected station → "View jobs" navigates; the list and status refetch.
the backend reason as a safety net.
Automated:
TestDeploymentSyncAll(5 tests — field,project_idrequired,permission matrix across all roles, anonymous denied, one-job-per-connected-
station) and the
parseServerErrorsuite (6 tests) pass. The data migrationapplies and reverses cleanly.
prettier,eslint, andtscare clean;makemigrations --checkreports no changes (the migration is a hand-written databackfill).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores