fix(notifications): dismiss race conditions

- Simplify when to focus the notifications panel for emergency notifications. - Try to limit race conditions when dismissing notifications.
aws · Nov 20, 2024 · 540ce54 · 540ce54
1 parent 4ea6d98
commit 540ce54
Show file tree

Hide file tree

Showing 2 changed files with 16 additions and 21 deletions.
diff --git a/packages/core/src/notifications/controller.ts b/packages/core/src/notifications/controller.ts
@@ -93,25 +93,20 @@ export class NotificationsController {
 
         await NotificationsNode.instance.setNotifications(startUp, emergency)
 
-        // Emergency notifications can't be dismissed, but if the user minimizes the panel then
-        // we don't want to focus it each time we set the notification nodes.
-        // So we store it in dismissed once a focus has been fired for it.
-        const newEmergencies = emergency.map((n) => n.id).filter((id) => !dismissed.has(id))
-        if (newEmergencies.length > 0) {
-            this.state.dismissed = [...this.state.dismissed, ...newEmergencies]
-            await this.writeState()
-            void this.notificationsNode.focusPanel()
-        }
-
         // Process on-receive behavior for newly received notifications that passes rule engine
-        const newlyReceivedToDisplay = [...startUp, ...emergency].filter((n) => this.state.newlyReceived.includes(n.id))
-        if (newlyReceivedToDisplay.length > 0) {
-            await this.notificationsNode.onReceiveNotifications(newlyReceivedToDisplay)
+        const wasNewlyReceived = (n: ToolkitNotification) => this.state.newlyReceived.includes(n.id)
+        const newStartUp = startUp.filter(wasNewlyReceived)
+        const newEmergency = emergency.filter(wasNewlyReceived)
+        const newlyReceived = [...newStartUp, ...newEmergency]
+
+        if (newlyReceived.length > 0) {
+            await this.notificationsNode.onReceiveNotifications(newlyReceived)
             // remove displayed notifications from newlyReceived
-            this.state.newlyReceived = this.state.newlyReceived.filter(
-                (id) => !newlyReceivedToDisplay.some((n) => n.id === id)
-            )
+            this.state.newlyReceived = this.state.newlyReceived.filter((id) => !newlyReceived.some((n) => n.id === id))
             await this.writeState()
+            if (newEmergency.length > 0) {
+                void this.notificationsNode.focusPanel()
+            }
         }
     }
 
@@ -201,7 +196,7 @@ export class NotificationsController {
      */
     private readState() {
         const state = this.getDefaultState()
-        this.state.dismissed = state.dismissed
+        this.state.dismissed = [...new Set([...this.state.dismissed, ...state.dismissed])]
     }
 
     /**

diff --git a/packages/core/src/test/notifications/controller.test.ts b/packages/core/src/test/notifications/controller.test.ts
@@ -160,7 +160,7 @@ describe('Notifications Controller', function () {
                 payload: content,
                 eTag,
             },
-            dismissed: [content.notifications[0].id],
+            dismissed: [],
             newlyReceived: ['id:emergency2'],
         })
         assert.equal(panelNode.startUpNotifications.length, 0)
@@ -218,7 +218,7 @@ describe('Notifications Controller', function () {
                 payload: emergencyContent,
                 eTag: eTag2,
             },
-            dismissed: [emergencyContent.notifications[0].id],
+            dismissed: [],
             newlyReceived: ['id:startup2', 'id:emergency2'],
         })
         assert.equal(panelNode.startUpNotifications.length, 1)
@@ -415,7 +415,7 @@ describe('Notifications Controller', function () {
                 payload: emergencyContent,
                 eTag: '1',
             },
-            dismissed: [emergencyContent.notifications[0].id, startUpContent.notifications[0].id],
+            dismissed: [startUpContent.notifications[0].id],
             newlyReceived: [],
         })
 
@@ -438,7 +438,7 @@ describe('Notifications Controller', function () {
                 payload: emergencyContent,
                 eTag: '1',
             },
-            dismissed: [emergencyContent.notifications[0].id],
+            dismissed: [],
             newlyReceived: [],
         })
         assert.equal(panelNode.getChildren().length, 1)