fix(settings): group connected services by deviceId to avoid name merging

sreecharan-desu · sreecharan-desu · commit 5b02a8bbc243 · 2026-02-12T10:05:01.000+05:30
diff --git a/packages/fxa-settings/src/components/Settings/ConnectedServices/index.test.tsx b/packages/fxa-settings/src/components/Settings/ConnectedServices/index.test.tsx
@@ -5,12 +5,7 @@
 import React from 'react';
 import { act, fireEvent, screen } from '@testing-library/react';
 import ConnectedServices, { sortAndFilterConnectedClients } from '.';
-import {
-  Account,
-  AlertBarInfo,
-  AppContext,
-  OAuthNativeClients,
-} from '../../../models';
+import { Account, AlertBarInfo, AppContext } from '../../../models';
 import {
   renderWithRouter,
   mockAppContext,
@@ -158,10 +153,13 @@ describe('Connected Services', () => {
       expect(result[result.length - 1]).toHaveTextContent('6 months ago');
     });
 
-    const { sortedAndUniqueClients, groupedByName } =
-      sortAndFilterConnectedClients(MOCK_SERVICES);
+    const { sortedAndUniqueClients } = sortAndFilterConnectedClients(MOCK_SERVICES);
+
+    const monitorClients = MOCK_SERVICES.filter(
+      (item) => item.name === 'Mozilla Monitor'
+    );
 
-    expect(sortedAndUniqueClients.length).toEqual(14);
+    expect(sortedAndUniqueClients.length).toEqual(13);
 
     expect(
       sortedAndUniqueClients.filter((item) => item.name === 'Mozilla Monitor')
@@ -172,7 +170,7 @@ describe('Connected Services', () => {
         (item) => item.name === 'Mozilla Monitor'
       )[0].lastAccessTime
     ).toEqual(1570736983000);
-    expect(groupedByName['Mozilla Monitor'].length).toEqual(2);
+      expect(monitorClients.length).toEqual(2);
   });
 
   it('should show the monitor icon and link', async () => {
@@ -314,7 +312,7 @@ describe('Connected Services', () => {
     );
     expect(
       await screen.findAllByTestId('connected-service-sign-out')
-    ).toHaveLength(14);
+    ).toHaveLength(13);
   });
 
   it('renders proper modal when "sign out" is clicked', async () => {
@@ -608,85 +606,4 @@ describe('Connected Services', () => {
       expect(mockWindowAssign).toHaveBeenCalledWith('foo-bar/signin');
     });
   });
-
-  describe('scope-based sub row', () => {
-    const baseMockClient = {
-      deviceId: null,
-      sessionTokenId: null,
-      refreshTokenId: 'abc123',
-      isCurrentSession: false,
-      deviceType: null,
-      createdTime: 1571412069000,
-      lastAccessTime: 1571412069000,
-      location: { city: null, country: null, state: null, stateCode: null },
-      userAgent: '',
-      os: null,
-      createdTimeFormatted: 'a month ago',
-      lastAccessTimeFormatted: 'a month ago',
-      approximateLastAccessTime: null,
-      approximateLastAccessTimeFormatted: null,
-    };
-
-    const renderWithClient = (client: Record<string, unknown>) => {
-      const account = {
-        attachedClients: [client],
-        disconnectClient: jest.fn().mockResolvedValue(true),
-      } as unknown as Account;
-      renderWithRouter(
-        <AppContext.Provider value={mockAppContext({ account })}>
-          <ConnectedServices />
-        </AppContext.Provider>
-      );
-    };
-
-    it('does not render scope sub-entry when scope is null and client ID is OAuthNative', async () => {
-      renderWithClient({
-        ...baseMockClient,
-        clientId: OAuthNativeClients.FirefoxDesktop,
-        name: 'Firefox Desktop',
-        scope: null,
-      });
-      await screen.findByTestId('settings-connected-service');
-      expect(screen.queryByTestId('scope-service')).not.toBeInTheDocument();
-    });
-
-    it('does not render scope sub-entry when scope has oldsync and profile but no relay', async () => {
-      renderWithClient({
-        ...baseMockClient,
-        clientId: OAuthNativeClients.FirefoxDesktop,
-        name: 'Firefox Desktop',
-        scope: ['https://identity.mozilla.com/apps/oldsync', 'profile'],
-      });
-      await screen.findByTestId('settings-connected-service');
-      expect(screen.queryByTestId('scope-service')).not.toBeInTheDocument();
-    });
-
-    it('renders Relay scope sub-entry when scope includes relay and client ID is OAuthNative', async () => {
-      renderWithClient({
-        ...baseMockClient,
-        clientId: OAuthNativeClients.FirefoxIOS,
-        name: 'Firefox for iOS',
-        scope: [
-          'https://identity.mozilla.com/apps/oldsync',
-          'profile',
-          'https://identity.mozilla.com/apps/relay',
-        ],
-      });
-      await screen.findByTestId('settings-connected-service');
-      const scopeEntry = screen.getByTestId('scope-service');
-      expect(scopeEntry).toBeInTheDocument();
-      expect(scopeEntry).toHaveTextContent('Firefox Relay');
-    });
-
-    it('does not render scope sub-entry when scope includes relay but client ID is not OAuthNative', async () => {
-      renderWithClient({
-        ...baseMockClient,
-        clientId: '9ebfe2c2f9ea3c58',
-        name: 'Firefox Relay',
-        scope: ['profile', 'https://identity.mozilla.com/apps/relay'],
-      });
-      await screen.findByTestId('settings-connected-service');
-      expect(screen.queryByTestId('scope-service')).not.toBeInTheDocument();
-    });
-  });
 });
diff --git a/packages/fxa-settings/src/components/Settings/ConnectedServices/index.tsx b/packages/fxa-settings/src/components/Settings/ConnectedServices/index.tsx
@@ -2,12 +2,14 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+import React from 'react';
+import { ApolloError } from '@apollo/client';
 import { Localized, useLocalization } from '@fluent/react';
 import { LinkExternal } from 'fxa-react/components/LinkExternal';
 import { useBooleanState } from 'fxa-react/lib/hooks';
 import groupBy from 'lodash.groupby';
 import { forwardRef, useCallback, useState } from 'react';
-import { clearSignedInAccountUid, setSigningOut } from '../../../lib/cache';
+import { clearSignedInAccountUid } from '../../../lib/cache';
 import { logViewEvent } from '../../../lib/metrics';
 import { isMobileDevice } from '../../../lib/utilities';
 import { AttachedClient, useAccount, useAlertBar } from '../../../models';
@@ -27,12 +29,12 @@ const DEVICES_SUPPORT_URL =
 export function sortAndFilterConnectedClients(
   attachedClients: Array<AttachedClient>
 ) {
-  const groupedByName = groupBy(attachedClients, 'name');
+  const groupedByDevice = groupBy(attachedClients, (c) => c.deviceId || c.name);
 
-  // get a unique (by name) list and sort by time last accessed
-  const sortedAndUniqueClients = Object.keys(groupedByName)
+  // get a unique (by device or name) list and sort by time last accessed
+  const sortedAndUniqueClients = Object.keys(groupedByDevice)
     .map((key) => {
-      return groupedByName[key].sort(
+      return groupedByDevice[key].sort(
         (a: AttachedClient, b: AttachedClient) =>
           b.lastAccessTime - a.lastAccessTime
       )[0];
@@ -47,14 +49,14 @@ export function sortAndFilterConnectedClients(
     }
   });
 
-  return { groupedByName, sortedAndUniqueClients };
+  return { groupedByDevice, sortedAndUniqueClients };
 }
 
 export const ConnectedServices = forwardRef<HTMLDivElement>((_, ref) => {
   const alertBar = useAlertBar();
   const account = useAccount();
   const attachedClients = account.attachedClients;
-  const { groupedByName, sortedAndUniqueClients } =
+  const { groupedByDevice, sortedAndUniqueClients } =
     sortAndFilterConnectedClients([...attachedClients]);
 
   const showMobilePromo = !sortedAndUniqueClients.filter(isMobileDevice).length;
@@ -82,7 +84,7 @@ export const ConnectedServices = forwardRef<HTMLDivElement>((_, ref) => {
   const [isRefreshingClients, setIsRefreshingClients] = useState(false);
 
   const clearDisconnectingState = useCallback(
-    (errorMessage?: string, error?: Error) => {
+    (errorMessage?: string, error?: ApolloError | Error) => {
       hideConfirmDisconnectModal();
       setSelectedClient(null);
       setReason('');
@@ -105,14 +107,12 @@ export const ConnectedServices = forwardRef<HTMLDivElement>((_, ref) => {
         // disconnect all clients/sessions with this name since only unique names
         // are displayed to the user. This is batched into one network request
         // via BatchHttpLink
-        const groupByKey = client.name ?? 'undefined';
-        const clientsWithMatchingName = groupedByName[groupByKey];
-        const hasMultipleSessions = clientsWithMatchingName.length > 1;
+        const groupByKey = (client.deviceId || client.name) ?? 'undefined';
+        const sessionsInGroup = groupedByDevice[groupByKey];
+        const hasMultipleSessions = sessionsInGroup.length > 1;
         if (hasMultipleSessions) {
           await Promise.all(
-            clientsWithMatchingName.map(
-              async (c) => await account.disconnectClient(c)
-            )
+            sessionsInGroup.map(async (c) => await account.disconnectClient(c))
           );
         } else {
           await account.disconnectClient(client);
@@ -123,9 +123,8 @@ export const ConnectedServices = forwardRef<HTMLDivElement>((_, ref) => {
         if (
           client.isCurrentSession ||
           (hasMultipleSessions &&
-            clientsWithMatchingName.find((c) => c.isCurrentSession))
+            sessionsInGroup.find((c) => c.isCurrentSession))
         ) {
-          setSigningOut(true);
           clearSignedInAccountUid();
           window.location.assign(`${window.location.origin}/signin`);
         } else if (reason === 'suspicious' || reason === 'lost') {
@@ -150,7 +149,7 @@ export const ConnectedServices = forwardRef<HTMLDivElement>((_, ref) => {
     [
       account,
       hideConfirmDisconnectModal,
-      groupedByName,
+      groupedByDevice,
       revealAdviceModal,
       alertBar,
       l10n,
@@ -228,7 +227,7 @@ export const ConnectedServices = forwardRef<HTMLDivElement>((_, ref) => {
         </div>
 
         {!!sortedAndUniqueClients.length &&
-          sortedAndUniqueClients.map((client) => (
+          sortedAndUniqueClients.map((client, i) => (
             <Service
               key={`${client.lastAccessTime}:${client.name || 'unknown'}`}
               {...{
@@ -238,7 +237,6 @@ export const ConnectedServices = forwardRef<HTMLDivElement>((_, ref) => {
                 lastAccessTimeFormatted: client.lastAccessTimeFormatted,
                 isCurrentSession: client.isCurrentSession,
                 clientId: client.clientId,
-                scope: client.scope,
                 handleSignOut: () => {
                   onSignOutClick(client);
                 },
