fix(devices): address release-PR review on device sync

apps/api/src/integration-platform/controllers/sync-available-providers.controller.spec.ts

@@ -11,13 +11,13 @@ import { GenericDeviceSyncService } from '../services/generic-device-sync.servic
 import { DynamicIntegrationRepository } from '../repositories/dynamic-integration.repository';
 import { CheckRunRepository } from '../repositories/check-run.repository';
 
-const mockConnectionFindFirst = jest.fn();
+const mockConnectionFindMany = jest.fn();
 const mockGetActiveManifests = jest.fn();
 
 jest.mock('@db', () => ({
   db: {
     integrationConnection: {
-      findFirst: (...args: unknown[]) => mockConnectionFindFirst(...args),
+      findMany: (...args: unknown[]) => mockConnectionFindMany(...args),
     },
   },
 }));
@@ -101,17 +101,15 @@ describe('SyncController - getAvailableSyncProviders connection status', () => {
   });
 
   it('reports connected + connectionStatus active for an active connection', async () => {
-    mockConnectionFindFirst.mockImplementation(
-      (args: { where: { status: string } }) =>
-        args.where.status === 'active'
-          ? {
-              id: 'icn_active',
-              status: 'active',
-              lastSyncAt: null,
-              nextSyncAt: null,
-            }
-          : null,
-    );
+    mockConnectionFindMany.mockResolvedValue([
+      {
+        id: 'icn_active',
+        status: 'active',
+        lastSyncAt: null,
+        nextSyncAt: null,
+        provider: { slug: 'intune' },
+      },
+    ]);
 
     const result = await controller.getAvailableSyncProviders(orgId, 'device');
 
@@ -123,15 +121,20 @@ describe('SyncController - getAvailableSyncProviders connection status', () => {
         connectionId: 'icn_active',
       }),
     ]);
-    // With an active connection there is no need to look up an errored one.
-    expect(mockConnectionFindFirst).toHaveBeenCalledTimes(1);
+    // ONE batched query for all providers — no per-provider point lookups.
+    expect(mockConnectionFindMany).toHaveBeenCalledTimes(1);
+    const args = mockConnectionFindMany.mock.calls[0][0] as {
+      where: { provider: { slug: { in: string[] } } };
+      orderBy: { updatedAt: string };
+    };
+    expect(args.where.provider.slug.in).toEqual(['intune']);
+    expect(args.orderBy).toEqual({ updatedAt: 'desc' });
   });
 
   it('reports connectionStatus error (not connected) when the latest connection is broken', async () => {
-    mockConnectionFindFirst.mockImplementation(
-      (args: { where: { status?: string } }) =>
-        args.where.status === 'active' ? null : { status: 'error' },
-    );
+    mockConnectionFindMany.mockResolvedValue([
+      { id: 'icn_broken', status: 'error', lastSyncAt: null, nextSyncAt: null, provider: { slug: 'intune' } },
+    ]);
 
     const result = await controller.getAvailableSyncProviders(orgId, 'device');
 
@@ -146,12 +149,12 @@ describe('SyncController - getAvailableSyncProviders connection status', () => {
   });
 
   it('does not resurface a stale error when the latest connection is disconnected', async () => {
-    // Reconnects create new rows, so an old 'error' row can coexist with a
-    // newer 'disconnected' one. The latest row (disconnected) must win.
-    mockConnectionFindFirst.mockImplementation(
-      (args: { where: { status?: string } }) =>
-        args.where.status === 'active' ? null : { status: 'disconnected' },
-    );
+    // Rows arrive newest-first (orderBy updatedAt desc): the user reconnected
+    // and later disconnected, leaving an older row stuck in 'error'.
+    mockConnectionFindMany.mockResolvedValue([
+      { id: 'icn_new', status: 'disconnected', lastSyncAt: null, nextSyncAt: null, provider: { slug: 'intune' } },
+      { id: 'icn_old', status: 'error', lastSyncAt: null, nextSyncAt: null, provider: { slug: 'intune' } },
+    ]);
 
     const result = await controller.getAvailableSyncProviders(orgId, 'device');
 
@@ -163,18 +166,28 @@ describe('SyncController - getAvailableSyncProviders connection status', () => {
         connectionId: null,
       }),
     ]);
-    // The fallback must pick the LATEST row regardless of status (no status
-    // filter), ordered by recency — not hunt for any old 'error' row.
-    const fallbackArgs = mockConnectionFindFirst.mock.calls[1][0] as {
-      where: { status?: string };
-      orderBy: { updatedAt: string };
-    };
-    expect(fallbackArgs.where.status).toBeUndefined();
-    expect(fallbackArgs.orderBy).toEqual({ updatedAt: 'desc' });
+  });
+
+  it('prefers an active connection even when a newer non-active row exists', async () => {
+    mockConnectionFindMany.mockResolvedValue([
+      { id: 'icn_new_err', status: 'error', lastSyncAt: null, nextSyncAt: null, provider: { slug: 'intune' } },
+      { id: 'icn_active', status: 'active', lastSyncAt: null, nextSyncAt: null, provider: { slug: 'intune' } },
+    ]);
+
+    const result = await controller.getAvailableSyncProviders(orgId, 'device');
+
+    expect(result.providers).toEqual([
+      expect.objectContaining({
+        slug: 'intune',
+        connected: true,
+        connectionStatus: 'active',
+        connectionId: 'icn_active',
+      }),
+    ]);
   });
 
   it('reports connectionStatus null when the org has no connection for the provider', async () => {
-    mockConnectionFindFirst.mockResolvedValue(null);
+    mockConnectionFindMany.mockResolvedValue([]);
 
     const result = await controller.getAvailableSyncProviders(orgId, 'device');
 

apps/api/src/integration-platform/controllers/sync.controller.ts

@@ -1777,53 +1777,55 @@ export class SyncController {
       m.capabilities?.includes(capability),
     );
 
-    const results = await Promise.all(
-      syncProviders.map(async (m) => {
-        const connection = await db.integrationConnection.findFirst({
-          where: {
-            organizationId,
-            status: 'active',
-            provider: { slug: m.id },
-          },
-          select: {
-            id: true,
-            status: true,
-            lastSyncAt: true,
-            nextSyncAt: true,
-          },
-        });
-        // No active connection: surface a broken (errored) one so the UI can
-        // prompt a reconnect instead of silently hiding sync for the provider.
-        // Multiple rows can exist per (org, provider) — reconnects create new
-        // rows — so the LATEST row decides: a newer 'disconnected' row means
-        // the user chose to disconnect, and a stale older 'error' row must
-        // not resurface a reconnect hint.
-        const latestConnection = connection
-          ? null
-          : await db.integrationConnection.findFirst({
-              where: {
-                organizationId,
-                provider: { slug: m.id },
-              },
-              select: { status: true },
-              orderBy: { updatedAt: 'desc' },
-            });
-        return {
-          slug: m.id,
-          name: m.name,
-          logoUrl: m.logoUrl,
-          connected: !!connection,
-          connectionStatus: connection
-            ? ('active' as const)
-            : latestConnection?.status === 'error'
-              ? ('error' as const)
-              : null,
-          connectionId: connection?.id ?? null,
-          lastSyncAt: connection?.lastSyncAt?.toISOString() ?? null,
-          nextSyncAt: connection?.nextSyncAt?.toISOString() ?? null,
-        };
-      }),
-    );
+    // One batched query for every provider's connections (newest first), then
+    // resolve per-provider status in memory — avoids N (to 2N) point lookups.
+    const orgConnections = await db.integrationConnection.findMany({
+      where: {
+        organizationId,
+        provider: { slug: { in: syncProviders.map((m) => m.id) } },
+      },
+      select: {
+        id: true,
+        status: true,
+        lastSyncAt: true,
+        nextSyncAt: true,
+        provider: { select: { slug: true } },
+      },
+      orderBy: { updatedAt: 'desc' },
+    });
+    const connectionsBySlug = new Map<string, typeof orgConnections>();
+    for (const conn of orgConnections) {
+      const slug = conn.provider.slug;
+      const list = connectionsBySlug.get(slug);
+      if (list) list.push(conn);
+      else connectionsBySlug.set(slug, [conn]);
+    }
+
+    const results = syncProviders.map((m) => {
+      const conns = connectionsBySlug.get(m.id) ?? [];
+      const connection = conns.find((c) => c.status === 'active');
+      // No active connection: surface a broken (errored) one so the UI can
+      // prompt a reconnect instead of silently hiding sync for the provider.
+      // Multiple rows can exist per (org, provider) — reconnects create new
+      // rows — so the LATEST row decides: a newer 'disconnected' row means
+      // the user chose to disconnect, and a stale older 'error' row must
+      // not resurface a reconnect hint.
+      const latestConnection = connection ? null : (conns[0] ?? null);
+      return {
+        slug: m.id,
+        name: m.name,
+        logoUrl: m.logoUrl,
+        connected: !!connection,
+        connectionStatus: connection
+          ? ('active' as const)
+          : latestConnection?.status === 'error'
+            ? ('error' as const)
+            : null,
+        connectionId: connection?.id ?? null,
+        lastSyncAt: connection?.lastSyncAt?.toISOString() ?? null,
+        nextSyncAt: connection?.nextSyncAt?.toISOString() ?? null,
+      };
+    });
 
     return { providers: results };
   }

apps/app/src/app/(app)/[orgId]/people/devices/components/DeviceSyncProviderSelector.test.tsx

@@ -81,6 +81,22 @@ describe('DeviceSyncProviderSelector — RBAC gating', () => {
     );
   });
 
+  it('marks the saved provider as selected in the open dropdown (controlled select)', async () => {
+    const user = userEvent.setup();
+    mockHasPermission.mockImplementation(
+      (resource: string, action: string) =>
+        resource === 'integration' && action === 'update',
+    );
+
+    render(<DeviceSyncProviderSelector />);
+
+    await user.click(
+      screen.getByRole('combobox', { name: /Sync devices from/i }),
+    );
+    const jamfOption = await screen.findByRole('option', { name: /Jamf/i });
+    expect(jamfOption).toHaveAttribute('aria-selected', 'true');
+  });
+
   it('renders nothing for a user without integration:update and disables the hook', () => {
     mockHasPermission.mockReturnValue(false);
 
@@ -114,7 +130,7 @@ describe('DeviceSyncProviderSelector — RBAC gating', () => {
     expect(screen.getByText('Not syncing')).toBeInTheDocument();
     await user.click(trigger);
     expect(
-      screen.getByRole('option', { name: /Kandji/i }),
+      await screen.findByRole('option', { name: /Kandji/i }),
     ).toBeInTheDocument();
     // The saved provider is still the user's choice — "Don't auto-sync" must
     // NOT be marked Active just because the choice can't be resolved to a
@@ -246,7 +262,9 @@ describe('DeviceSyncProviderSelector — connection states', () => {
     mockHook({
       selectedProvider: null,
       availableProviders: [
-        { ...provider, connected: false, connectionId: null },
+        // Explicitly undefined — the base fixture carries 'active', which
+        // would not represent an older API response lacking the field.
+        { ...provider, connected: false, connectionStatus: undefined, connectionId: null },
       ],
       hasAnyConnection: false,
     });

apps/app/src/app/(app)/[orgId]/people/devices/components/DeviceSyncProviderSelector.tsx

@@ -104,8 +104,22 @@ export function DeviceSyncProviderSelector() {
     // the dropdown's info block.
     <div className="flex flex-wrap items-center justify-end gap-2">
       <div className="w-full max-w-[240px]">
-        {/* Uncontrolled on purpose — mirrors the (working) people-sync select. */}
-        <Select onValueChange={handleValueChange} disabled={isSyncing}>
+        {/* Controlled: unlike the popover-hosted people-sync selects (which
+            stay uncontrolled to survive popover dismissal), this renders
+            directly on the page — passing value keeps the saved selection
+            checked in the open dropdown for keyboard and screen-reader users.
+            Guarded to values that exist as items: a saved provider that is no
+            longer listed would otherwise break the select. */}
+        <Select
+          value={
+            connectedProviders.some((p) => p.slug === selectedProvider) ||
+            erroredProviders.some((p) => p.slug === selectedProvider)
+              ? selectedProvider
+              : null
+          }
+          onValueChange={handleValueChange}
+          disabled={isSyncing}
+        >
           <SelectTrigger aria-label="Sync devices from">
             {isSyncing ? (
               <div className="flex items-center gap-2">