Skip to content

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

Open
mihow wants to merge 11 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 11 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 can edit. New SyncDeploymentDialog in the actions Toolbar of deployment-columns.tsx, gated on item.canUpdate && 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 canUpdate || canSync. canSync maps to the
    sync permission (ML data managers, Project managers). The canUpdate arm is
    there for superusers: they receive update/delete in user_permissions but
    not sync (guardian returns no object perms for superusers), so canUpdate
    keeps the buttons visible for them.
  • 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. 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):

  • 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 (5 tests — field, project_id required,
permission matrix across all roles, anonymous denied, one-job-per-connected-
station) 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 all eligible deployments in a project.
    • Showed whether a deployment is connected to a data source, enabling clearer action visibility.
    • Added links to the created job after a sync starts.
  • Bug Fixes

    • Improved error handling so validation messages display correctly for array-based server responses.
    • Prevented sync options from appearing for deployments without a connected data source.
  • Chores

    • Updated permissions so the appropriate roles can use 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 canceled.

Name Link
🔨 Latest commit 7f8e318
🔍 Latest deploy log https://app.netlify.com/projects/antenna-preview/deploys/6a46cd8d9f081e0008dfccbf

@netlify

netlify Bot commented Jun 30, 2026

Copy link
Copy Markdown

Deploy Preview for antenna-ssec canceled.

Name Link
🔨 Latest commit 7f8e318
🔍 Latest deploy log https://app.netlify.com/projects/antenna-ssec/deploys/6a46cd8de8fe8c0008e6c970

@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@mihow, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 29 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d793d9fd-40d0-4596-9cc3-bdaca51d26a9

📥 Commits

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

📒 Files selected for processing (6)
  • ami/main/migrations/0095_grant_sync_deployment_to_mldatamanager.py
  • ami/main/tests.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/pages/deployments/sync-all-deployments-dialog.tsx
  • ui/src/pages/deployments/sync-deployment-dialog.tsx
📝 Walkthrough

Walkthrough

This PR adds a bulk "sync-all" deployment endpoint alongside the existing per-deployment sync action, a data_source_connected serializer flag, an MLDataManager sync permission grant with backfill migration, corresponding frontend hooks and dialogs, a parseServerError array-handling fix, tests, and planning documentation.

Changes

Sync Feature Implementation

Layer / File(s) Summary
Serializer flag, permission grant, and migration
ami/main/api/serializers.py, ami/users/roles.py, ami/main/migrations/0095_grant_sync_deployment_to_mldatamanager.py
Adds data_source_connected to DeploymentListSerializer, grants SYNC_DEPLOYMENT to MLDataManager, and backfills the permission onto existing groups.
Sync and sync-all API actions
ami/main/api/views.py
Adds a shared _enqueue_sync_job helper used by both the per-deployment sync action and the new bulk sync-all action, which enqueues jobs for all connected project deployments.
Backend tests
ami/main/tests.py
Adds TestDeploymentSyncAll covering serialization, validation, permissions, and job enqueue behavior.
Frontend permission/type additions
ui/src/utils/user/types.ts, ui/src/data-services/models/deployment.ts
Adds UserPermission.Sync and canSync/dataSourceConnected getters on Deployment.
parseServerError array handling
ui/src/utils/parseServerError/*
Handles array-shaped error payloads by joining into a message string, with a new test.
Sync mutation hooks
ui/src/data-services/hooks/deployments/*
Adds useSyncAllDeployments and exposes reset from useSyncDeploymentSourceImages.
New sync UI strings
ui/src/utils/language.ts
Adds enum keys and English text for sync/sync-all labels and confirmations.
Sync dialogs
ui/src/pages/deployments/sync-deployment-dialog.tsx, ui/src/pages/deployments/sync-all-deployments-dialog.tsx
Implements confirmation dialogs with loading/success states, error display, and job links.
Deployments list wiring
ui/src/pages/deployments/deployment-columns.tsx, ui/src/pages/deployments/deployments.tsx
Wires dialogs into the table actions column and page header, gated on permissions and connected data source.

Estimated code review effort: 3 (Moderate) | ~30 minutes

Planning Documentation

Layer / File(s) Summary
Station sync planning docs
docs/claude/planning/2026-06-29-station-sync-action-*.md
Adds planning/design markdown documenting shipped and follow-up sync behavior.

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
Loading

Suggested labels: PSv2

Suggested reviewers: annavik

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding per-station and bulk sync actions from the stations list.
Description check ✅ Passed The description covers summary, changes, screenshots, testing, and permissions, with only non-critical template sections missing.
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.
✨ 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
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