Merge pull request #31500 from backstage/fix/signals-subscribe-twice-on-error

Fix: signals subscribing twice on error

.changeset/quiet-singers-pick.md

@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-signals': patch
+---
+
+Fixes a bug where the `SignalClient` would try to subscribe to the same channel twice after an error, instead of just once.

plugins/signals/package.json

@@ -71,7 +71,8 @@
     "jest-websocket-mock": "^2.5.0",
     "react": "^18.0.2",
     "react-dom": "^18.0.2",
-    "react-router-dom": "^6.3.0"
+    "react-router-dom": "^6.3.0",
+    "wait-for-expect": "^3.0.2"
   },
   "peerDependencies": {
     "@types/react": "^17.0.0 || ^18.0.0",

plugins/signals/src/api/SignalClient.ts

@@ -176,7 +176,10 @@ export class SignalClient implements SignalApi {
     };
 
     this.ws.onerror = () => {
-      this.reconnect();
+      if (this.ws) {
+        this.ws.close();
+      }
+      this.ws = null;
     };
 
     this.ws.onclose = (ev: CloseEvent) => {

plugins/signals/src/api/SignalsClient.test.ts

@@ -17,8 +17,9 @@
 import { mockApis } from '@backstage/test-utils';
 import WS from 'jest-websocket-mock';
 import { SignalClient } from './SignalClient';
+import waitForExpect from 'wait-for-expect';
 
-describe('SignalsClient', () => {
+describe('SignalClient', () => {
   const identity = mockApis.identity({ token: '12345' });
   const discoveryApi = mockApis.discovery({ baseUrl: 'http://localhost:1234' });
 
@@ -72,25 +73,51 @@ describe('SignalsClient', () => {
 
     await server.connected;
 
-    await expect(server).toReceiveMessage({
-      action: 'subscribe',
-      channel: 'channel',
-    });
+    await waitForExpect(() =>
+      expect(server).toHaveReceivedMessages([
+        {
+          action: 'subscribe',
+          channel: 'channel',
+        },
+        {
+          action: 'subscribe',
+          channel: 'channel',
+        },
+      ]),
+    );
     server.send({ channel: 'channel', message: { hello: 'world' } });
     expect(messageMock1).toHaveBeenCalledWith({ hello: 'world' });
     expect(messageMock2).toHaveBeenCalledWith({ hello: 'world' });
 
     await unsubscribe1();
-    await expect(server).not.toReceiveMessage({
-      action: 'unsubscribe',
-      channel: 'channel',
-    });
+    await waitForExpect(() =>
+      expect(server).toReceiveMessage({
+        action: 'unsubscribe',
+        channel: 'channel',
+      }),
+    );
 
     await unsubscribe2();
-    await expect(server).toReceiveMessage({
-      action: 'unsubscribe',
-      channel: 'channel',
-    });
+    await waitForExpect(() =>
+      expect(server.messages).toEqual([
+        {
+          action: 'subscribe',
+          channel: 'channel',
+        },
+        {
+          action: 'subscribe',
+          channel: 'channel',
+        },
+        {
+          action: 'unsubscribe',
+          channel: 'channel',
+        },
+        {
+          action: 'unsubscribe',
+          channel: 'channel',
+        },
+      ]),
+    );
   });
 
   it('should reconnect on error', async () => {
@@ -111,10 +138,11 @@ describe('SignalsClient', () => {
 
     await server.server.emit('error', null);
 
-    await new Promise(r => setTimeout(r, 50));
-    await expect(server).toReceiveMessage({
-      action: 'subscribe',
-      channel: 'channel',
-    });
+    await waitForExpect(() =>
+      expect(server.messages).toEqual([
+        { action: 'subscribe', channel: 'channel' },
+        { action: 'subscribe', channel: 'channel' },
+      ]),
+    );
   });
 });

yarn.lock

@@ -7421,6 +7421,7 @@ __metadata:
     react-dom: "npm:^18.0.2"
     react-router-dom: "npm:^6.3.0"
     uuid: "npm:^11.0.0"
+    wait-for-expect: "npm:^3.0.2"
   peerDependencies:
     "@types/react": ^17.0.0 || ^18.0.0
     react: ^17.0.0 || ^18.0.0