Merge pull request #6052 from hayemaxi/notifications6

fix(notifications): extension activation hangs if fetch fails

packages/amazonq/src/extensionNode.ts

@@ -75,7 +75,7 @@ async function activateAmazonQNode(context: vscode.ExtensionContext) {
         ...authState,
     })
 
-    await activateNotifications(context, authState, getAuthState)
+    void activateNotifications(context, authState, getAuthState)
 }
 
 async function getAuthState(): Promise<Omit<AuthUserState, 'source'>> {

packages/core/src/extensionNode.ts

@@ -246,7 +246,7 @@ export async function activate(context: vscode.ExtensionContext) {
             ...authState,
         })
 
-        await activateNotifications(context, authState, getAuthState)
+        void activateNotifications(context, authState, getAuthState)
     } catch (error) {
         const stacktrace = (error as Error).stack?.split('\n')
         // truncate if the stacktrace is unusually long

packages/core/src/notifications/activation.ts

@@ -13,6 +13,8 @@ import { AuthState } from './types'
 import { getLogger } from '../shared/logger/logger'
 import { oneMinute } from '../shared/datetime'
 
+const logger = getLogger('notifications')
+
 /** Time in MS to poll for emergency notifications */
 const emergencyPollTime = oneMinute * 10
 
@@ -33,19 +35,23 @@ export async function activate(
         return
     }
 
-    const panelNode = NotificationsNode.instance
-    panelNode.registerView(context)
+    try {
+        const panelNode = NotificationsNode.instance
+        panelNode.registerView(context)
 
-    const controller = new NotificationsController(panelNode)
-    const engine = new RuleEngine(await getRuleContext(context, initialState))
+        const controller = new NotificationsController(panelNode)
+        const engine = new RuleEngine(await getRuleContext(context, initialState))
 
-    await controller.pollForStartUp(engine)
-    await controller.pollForEmergencies(engine)
+        await controller.pollForStartUp(engine)
+        await controller.pollForEmergencies(engine)
 
-    globals.clock.setInterval(async () => {
-        const ruleContext = await getRuleContext(context, await authStateFn())
-        await controller.pollForEmergencies(new RuleEngine(ruleContext))
-    }, emergencyPollTime)
+        globals.clock.setInterval(async () => {
+            const ruleContext = await getRuleContext(context, await authStateFn())
+            await controller.pollForEmergencies(new RuleEngine(ruleContext))
+        }, emergencyPollTime)
 
-    getLogger('notifications').debug('Activated in-IDE notifications polling module')
+        logger.debug('Activated in-IDE notifications polling module')
+    } catch (err) {
+        logger.error('Failed to activate in-IDE notifications module.')
+    }
 }

packages/core/src/notifications/controller.ts

@@ -26,6 +26,8 @@ import { FileResourceFetcher } from '../shared/resourcefetcher/fileResourceFetch
 import { isAmazonQ } from '../shared/extensionUtilities'
 import { telemetry } from '../shared/telemetry/telemetry'
 
+const logger = getLogger('notifications')
+
 /**
  * Handles fetching and maintaining the state of in-IDE notifications.
  * Notifications are constantly polled from a known endpoint and then stored in global state.
@@ -39,8 +41,6 @@ import { telemetry } from '../shared/telemetry/telemetry'
  * Emergency notifications - fetched at a regular interval.
  */
 export class NotificationsController {
-    public static readonly suggestedPollIntervalMs = 1000 * 60 * 10 // 10 minutes
-
     /** Internal memory state that is written to global state upon modification. */
     private readonly state: NotificationsState
 
@@ -75,7 +75,7 @@ export class NotificationsController {
             this.readState()
             await this.fetchNotifications(category)
         } catch (err: any) {
-            getLogger('notifications').error(`Unable to fetch %s notifications: %s`, category, err)
+            logger.error(`Unable to fetch %s notifications: %s`, category, err)
         }
 
         await this.displayNotifications(ruleEngine)
@@ -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()
+            }
         }
     }
 
@@ -121,7 +116,9 @@ export class NotificationsController {
      * hide all notifications.
      */
     public async dismissNotification(notificationId: string) {
-        getLogger('notifications').debug('Dismissing notification: %s', notificationId)
+        logger.debug('Dismissing notification: %s', notificationId)
+
+        this.readState() // Don't overwrite dismissals from other windows
         this.state.dismissed.push(notificationId)
         await this.writeState()
 
@@ -134,7 +131,7 @@ export class NotificationsController {
     private async fetchNotifications(category: NotificationType) {
         const response = await this.fetcher.fetch(category, this.state[category].eTag)
         if (!response.content) {
-            getLogger('notifications').verbose('No new notifications for category: %s', category)
+            logger.verbose('No new notifications for category: %s', category)
             return
         }
         // Parse the notifications
@@ -149,7 +146,7 @@ export class NotificationsController {
         const addedNotifications = newNotifications.filter((n: any) => !currentNotificationIds.has(n.id))
 
         if (addedNotifications.length > 0) {
-            getLogger('notifications').verbose(
+            logger.verbose(
                 'New notifications received for category %s, ids: %s',
                 category,
                 addedNotifications.map((n: any) => n.id).join(', ')
@@ -161,7 +158,7 @@ export class NotificationsController {
         this.state[category].eTag = response.eTag
         await this.writeState()
 
-        getLogger('notifications').verbose(
+        logger.verbose(
             "Fetched notifications JSON for category '%s' with schema version: %s. There were %d notifications.",
             category,
             this.state[category].payload?.schemaVersion,
@@ -173,7 +170,7 @@ export class NotificationsController {
      * Write the latest memory state to global state.
      */
     private async writeState() {
-        getLogger('notifications').debug('NotificationsController: Updating notifications state at %s', this.storageKey)
+        logger.debug('NotificationsController: Updating notifications state at %s', this.storageKey)
 
         // Clean out anything in 'dismissed' that doesn't exist anymore.
         const notifications = new Set(
@@ -199,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])]
     }
 
     /**
@@ -239,7 +236,7 @@ function registerDismissCommand() {
                     await NotificationsController.instance.dismissNotification(notification.id)
                 })
             } else {
-                getLogger('notifications').error(`${name}: Cannot dismiss notification: item is not a vscode.TreeItem`)
+                logger.error(`${name}: Cannot dismiss notification: item is not a vscode.TreeItem`)
             }
         })
     )
@@ -277,17 +274,23 @@ export class RemoteFetcher implements NotificationFetcher {
         const fetcher = new HttpResourceFetcher(endpoint, {
             showUrl: true,
         })
-        getLogger('notifications').verbose(
-            'Attempting to fetch notifications for category: %s at endpoint: %s',
-            category,
-            endpoint
+        logger.verbose('Attempting to fetch notifications for category: %s at endpoint: %s', category, endpoint)
+
+        return withRetries(
+            async () => {
+                try {
+                    return await fetcher.getNewETagContent(versionTag)
+                } catch (err) {
+                    logger.error('Failed to fetch at endpoint: %s, err: %s', endpoint, err)
+                    throw err
+                }
+            },
+            {
+                maxRetries: RemoteFetcher.retryNumber,
+                delay: RemoteFetcher.retryIntervalMs,
+                // No exponential backoff - necessary?
+            }
         )
-
-        return withRetries(async () => await fetcher.getNewETagContent(versionTag), {
-            maxRetries: RemoteFetcher.retryNumber,
-            delay: RemoteFetcher.retryIntervalMs,
-            // No exponential backoff - necessary?
-        })
     }
 }
 
@@ -309,11 +312,7 @@ export class LocalFetcher implements NotificationFetcher {
 
     async fetch(category: NotificationType, versionTag?: string): Promise<ResourceResponse> {
         const uri = category === 'startUp' ? this.startUpLocalPath : this.emergencyLocalPath
-        getLogger('notifications').verbose(
-            'Attempting to fetch notifications locally for category: %s at path: %s',
-            category,
-            uri
-        )
+        logger.verbose('Attempting to fetch notifications locally for category: %s at path: %s', category, uri)
 
         return {
             content: await new FileResourceFetcher(globals.context.asAbsolutePath(uri)).get(),

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)