Skip to content

Sync station storage sources from the stations list, per station or all at once#1357

Open
mihow wants to merge 13 commits into
mainfrom
feat/station-sync-action
Open

Sync station storage sources from the stations list, per station or all at once#1357
mihow wants to merge 13 commits into
mainfrom
feat/station-sync-action

Conversation

@mihow

@mihow mihow commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

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

Per-row confirm Per-row error (no storage source)
Sync confirmation dialog Error when a station has no storage source
Sync all Stations list header
Sync all dialog after success Stations list with the Sync all button

List of Changes

Change (user effect) How (implementation)
A "Sync" button appears on each station row (beside Delete) that has a storage source and that the user is allowed to sync. New SyncDeploymentDialog in the actions Toolbar of deployment-columns.tsx, gated on item.canSync && item.dataSourceConnected. Reuses useSyncDeploymentSourceImages (POST /deployments/{id}/sync/).
A "Sync all" button in the Stations list header syncs every connected station the user can edit; the dialog says how many jobs will run and links to the Jobs tab on success. New SyncAllDeploymentsDialog + useSyncAllDeployments hook (POST /deployments/sync-all/?project_id=). The header shows it only when at least one station is syncable.
The stations list reports whether each station has a storage source. New read-only data_source_connected boolean on DeploymentListSerializer (derived from the data_source FK id, no extra query); surfaced as Deployment.dataSourceConnected on the frontend.
"Sync all" enqueues one background job per connected station — separate jobs, not one combined job. New DeploymentViewSet.sync_all action. It requires project_id, checks the sync permission itself (a detail=False action is not covered by ObjectPermission.has_object_permission), and enqueues one DataStorageSyncJob per connected station, mirroring the admin bulk action.
ML data managers can now sync stations, not only Project managers. Granted sync_deployment to the MLDataManager role (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.
Syncing a station with no storage source explains why ("Deployment must have a data source to sync captures from") instead of a generic "Request failed with status code 400". The endpoint raises a plain-string DRF ValidationError (serialized as a top-level JSON list); parseServerError now reads that shape. Benefits every endpoint that raises a plain-string error.
The dialog's error banner renders above the title instead of overlapping it. Dropped the inDialog (sticky, header-offset) variant of FormError, matching DeleteForm.
Reopening a dialog offers a fresh sync rather than the previous attempt's stale result. The sync hooks expose reset(), called on dialog open (the hooks stay mounted with the row / header).
New UI copy is translatable. SYNC, SYNC_ALL, SYNC_CAPTURES, MESSAGE_SYNC_CONFIRM, MESSAGE_SYNC_ALL_CONFIRM, VIEW_JOB, VIEW_JOBS in ui/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_deployment permission, which was
Project-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)

  • The frontend gates the buttons on canSync — the sync entry in a
    deployment's user_permissions, backed by the sync_deployment permission
    (ML data managers, Project managers, and superusers). Superusers receive
    sync here because guardian returns every permission defined on the project's
    content type for a superuser. Gating on canUpdate instead would be wrong:
    update_deployment is Project-manager-only, so it would hide the button from
    ML data managers who can sync, and it would show a dead button to any future
    role granted update_deployment without sync_deployment.
  • sync_all is a detail=False action, which ObjectPermission does not guard
    (its has_object_permission only runs for detail actions via get_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. Permission-matrix tests cover both the per-row and the bulk
    endpoints: superuser / ProjectManager / MLDataManager (200) and Researcher /
    Identifier / BasicMember / non-member (403).

Concurrency

"Sync all" deliberately enqueues separate jobs and does not deduplicate against
syncs that may already be running, because duplicate syncs on the same station
cannot corrupt data. Event regrouping is serialized by the per-deployment cache
lock in group_images_into_events (the "sync-time regroup" caller named in its
docstring, added after the workshop-triage race), and source-image insertion
upserts on (deployment, path). A duplicate "Sync all" run therefore only costs
redundant work and extra job rows, not incorrect state. A skip-if-already-running
guard would be a nicety, not a correctness fix, and would apply equally to the
existing per-row and admin bulk actions.

Verification

Verified against a live docker-compose stack with Chrome DevTools (a project with
one connected and one unconnected station):

  • Per-row Sync shows only on the connected station; confirm → POST /deployments/{id}/sync/ 200 → job created → "View job" navigates.
  • "Sync all" → POST /deployments/sync-all/?project_id= 200 → one job per
    connected station → "View jobs" navigates; the list and status refetch.
  • The unconnected station has no per-row Sync button; the endpoint still returns
    the backend reason as a safety net.
  • Reopening either dialog after success/error offers a fresh sync.

Automated: TestDeploymentSyncAll (6 tests — field, project_id required,
per-row and bulk permission matrices across all roles, anonymous denied,
one-job-per-connected-station), TestSyncDeploymentBackfillMigration, and the
parseServerError suite (6 tests) pass. The data migration applies and reverses
cleanly. prettier, eslint, and tsc are clean;
makemigrations --check reports no changes (the migration is a hand-written data
backfill).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added sync actions for individual deployments and for eligible deployments in a project.
    • Show sync availability based on whether a deployment is connected to a data source.
    • Added new sync UI dialogs (including a bulk “sync all” flow) and links to the enqueued job when sync starts.
  • Bug Fixes

    • Improved user-facing error handling for array-based server responses.
    • Hides sync options for deployments without a connected data source.
  • Chores

    • Updated permissions and added a backfill migration to ensure the right roles can run sync actions.

mihow and others added 3 commits June 29, 2026 22:27
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>
@netlify

netlify Bot commented Jun 30, 2026

Copy link
Copy Markdown

Deploy Preview for antenna-preview ready!

Name Link
🔨 Latest commit 0d60a1e
🔍 Latest deploy log https://app.netlify.com/projects/antenna-preview/deploys/6a5001052c918a000877550e
😎 Deploy Preview https://deploy-preview-1357--antenna-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 53 (🔴 down 12 from production)
Accessibility: 81 (🔴 down 8 from production)
Best Practices: 92 (🔴 down 8 from production)
SEO: 92 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports
🤖 Make changes Run an agent on this branch

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

@netlify

netlify Bot commented Jun 30, 2026

Copy link
Copy Markdown

Deploy Preview for antenna-ssec ready!

Name Link
🔨 Latest commit 0d60a1e
🔍 Latest deploy log https://app.netlify.com/projects/antenna-ssec/deploys/6a50010506669000085a2e5e
😎 Deploy Preview https://deploy-preview-1357--antenna-ssec.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

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

@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4d731f2c-3fb6-4dd5-ab73-92c01388798a

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

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR adds per-deployment and bulk deployment sync actions, exposes deployment connection state in the API, updates permissions and migration backfill, and wires new frontend dialogs, hooks, labels, and tests for the sync flow.

Changes

Sync Feature Implementation

Layer / File(s) Summary
Serializer and permission changes
ami/main/api/serializers.py, ami/users/roles.py, ami/main/migrations/0095_grant_sync_deployment_to_mldatamanager.py
Adds data_source_connected, grants sync permission to MLDataManager, and backfills group/object permissions.
Sync endpoints
ami/main/api/views.py
Shares sync-job enqueueing between per-deployment sync and bulk sync-all actions.
Backend tests
ami/main/tests.py
Covers sync-all behavior, permission cases, and migration backfill behavior.
Frontend support data and hooks
ui/src/utils/user/types.ts, ui/src/data-services/models/deployment.ts, ui/src/data-services/hooks/deployments/*, ui/src/utils/parseServerError/*, ui/src/utils/language.ts
Adds sync permission/model fields, sync hooks, error parsing for array payloads, and sync UI strings.
Dialogs and page wiring
ui/src/pages/deployments/*
Adds sync dialogs and conditionally renders them from the deployments list and page header.
Planning documents
docs/claude/planning/2026-06-29-station-sync-action-*.md
Adds planning/status docs for the shipped sync action and related follow-up notes.

Estimated code review effort: 4 (Complex) | ~60 minutes

Suggested labels: PSv2

Suggested reviewers: annavik

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main feature: station storage-source syncing from the list, individually or in bulk.
Description check ✅ Passed The description is detailed and mostly matches the template, covering summary, changes, screenshots, testing, and deployment notes.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/station-sync-action

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.

mihow and others added 4 commits July 1, 2026 08:30
…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>
@mihow mihow changed the title Sync a station's storage source from the stations list Sync station storage sources from the stations list, per station or all at once Jul 1, 2026
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>
@mihow mihow marked this pull request as ready for review July 2, 2026 20:13
Copilot AI review requested due to automatic review settings July 2, 2026 20:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_connected to the deployments list serializer and introduces a bulk POST /deployments/sync-all/?project_id= endpoint that enqueues one sync job per connected station.
  • Permissions: Grants sync_deployment to 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 assigns details directly to message, which can make message an 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.

Comment thread ami/main/migrations/0095_grant_sync_deployment_to_mldatamanager.py Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

♻️ Duplicate comments (1)
ui/src/pages/deployments/sync-all-deployments-dialog.tsx (1)

27-36: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Same 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(), clearing isLoading/isSuccess before 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 in ui/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 value

Consider 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 win

Duplicate eligibility predicate for sync UI.

The (item.canUpdate || item.canSync) && item.dataSourceConnected check here is duplicated verbatim in ui/src/pages/deployments/deployments.tsx (syncableCount filter). Consider exposing a single canSyncNow-style getter on the Deployment model 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

📥 Commits

Reviewing files that changed from the base of the PR and between cdcb3b1 and 25318a8.

📒 Files selected for processing (18)
  • ami/main/api/serializers.py
  • ami/main/api/views.py
  • ami/main/migrations/0095_grant_sync_deployment_to_mldatamanager.py
  • ami/main/tests.py
  • ami/users/roles.py
  • docs/claude/planning/2026-06-29-station-sync-action-NEXT.md
  • docs/claude/planning/2026-06-29-station-sync-action-design.md
  • ui/src/data-services/hooks/deployments/useSyncAllDeployments.ts
  • ui/src/data-services/hooks/deployments/useSyncDeploymentSourceImages.ts
  • ui/src/data-services/models/deployment.ts
  • ui/src/pages/deployments/deployment-columns.tsx
  • ui/src/pages/deployments/deployments.tsx
  • ui/src/pages/deployments/sync-all-deployments-dialog.tsx
  • ui/src/pages/deployments/sync-deployment-dialog.tsx
  • ui/src/utils/language.ts
  • ui/src/utils/parseServerError/parseServerError.test.ts
  • ui/src/utils/parseServerError/parseServerError.ts
  • ui/src/utils/user/types.ts

Comment thread ami/main/migrations/0095_grant_sync_deployment_to_mldatamanager.py Outdated
Comment thread docs/claude/planning/2026-06-29-station-sync-action-design.md Outdated
Comment thread docs/claude/planning/2026-06-29-station-sync-action-NEXT.md Outdated
Comment thread ui/src/pages/deployments/sync-deployment-dialog.tsx
mihow and others added 3 commits July 2, 2026 13:37
…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>
@mihow mihow force-pushed the feat/station-sync-action branch from 9e4f555 to 7f8e318 Compare July 2, 2026 20:43
@mihow mihow requested a review from annavik July 9, 2026 10:47
mihow and others added 2 commits July 9, 2026 13:13
The design addendum and handoff doc claimed superusers do not receive the
`sync` permission and that the button is gated on `canUpdate`. Both are wrong:
guardian returns every project permission for superusers, so they do get
`sync`, and the button is gated on `canSync`. Correct the addendum, and trim
the handoff doc to a scrubbed status summary (drop local paths and the
session repair note).

Co-Authored-By: Claude <noreply@anthropic.com>
The per-row and "Sync all" buttons were gated on `canUpdate || canSync`. The
`canUpdate` arm was added on the mistaken belief that superusers do not receive
the `sync` permission; in fact guardian returns every project permission for
superusers, so `canSync` already covers them. The OR gate is not only redundant
but a latent bug: any future role granted `update_deployment` without
`sync_deployment` would see a sync button that 403s on click. Gate on `canSync`
alone, the exact set of users the endpoint permits (ML data managers, project
managers, superusers).

Add a per-row sync permission matrix test; previously only the bulk endpoint
was covered, though both paths share the same `sync_deployment` check.

Co-Authored-By: Claude <noreply@anthropic.com>
@mihow

mihow commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants