feat(notification): misc logging, types, fixes

packages/core/src/notifications/controller.ts

@@ -40,24 +40,22 @@ import { telemetry } from '../shared/telemetry/telemetry'
 export class NotificationsController {
     public static readonly suggestedPollIntervalMs = 1000 * 60 * 10 // 10 minutes
 
-    public readonly storageKey: globalKey
-
     /** Internal memory state that is written to global state upon modification. */
     private readonly state: NotificationsState
 
     static #instance: NotificationsController | undefined
 
     constructor(
         private readonly notificationsNode: NotificationsNode,
-        private readonly fetcher: NotificationFetcher = new RemoteFetcher()
+        private readonly fetcher: NotificationFetcher = new RemoteFetcher(),
+        public readonly storageKey: globalKey = 'aws.notifications'
     ) {
         if (!NotificationsController.#instance) {
             // Register on first creation only.
             registerDismissCommand()
         }
         NotificationsController.#instance = this
 
-        this.storageKey = 'aws.notifications'
         this.state = globals.globalState.tryGet<NotificationsState>(this.storageKey, NotificationsStateConstructor, {
             startUp: {},
             emergency: {},

packages/core/src/notifications/panelNode.ts

@@ -78,24 +78,23 @@ export class NotificationsNode implements TreeNode {
     }
 
     public refresh(): void {
-        if (!this.view) {
-            throw new ToolkitError('NotificationsNode: TreeView accessed without being registered.')
-        }
-
         const totalNotifications = this.notificationCount()
-        if (totalNotifications > 0) {
-            this.view.badge = {
-                tooltip: `${totalNotifications} notification${totalNotifications > 1 ? 's' : ''}`,
-                value: totalNotifications,
+        if (this.view) {
+            if (totalNotifications > 0) {
+                this.view.badge = {
+                    tooltip: `${totalNotifications} notification${totalNotifications > 1 ? 's' : ''}`,
+                    value: totalNotifications,
+                }
+                this.view.title = `${NotificationsNode.title} (${totalNotifications})`
+            } else {
+                this.view.badge = undefined
+                this.view.title = NotificationsNode.title
             }
-            this.view.title = `${NotificationsNode.title} (${totalNotifications})`
-            void setContext(this.showContextStr, true)
         } else {
-            this.view.badge = undefined
-            this.view.title = NotificationsNode.title
-            void setContext(this.showContextStr, false)
+            logger.warn('NotificationsNode was refreshed but the view was not initialized!')
         }
 
+        void setContext(this.showContextStr, totalNotifications > 0)
         this.provider?.refresh()
     }
 
@@ -240,7 +239,7 @@ export class NotificationsNode implements TreeNode {
                                 case 'updateAndReload':
                                     // Give things time to finish executing.
                                     globals.clock.setTimeout(() => {
-                                        void this.updateAndReload(notification.displayIf.extensionId)
+                                        return this.updateAndReload(notification.displayIf.extensionId)
                                     }, 1000)
                                     break
                                 case 'openUrl':

packages/core/src/test/notifications/controller.test.ts

@@ -8,7 +8,8 @@ import * as FakeTimers from '@sinonjs/fake-timers'
 import assert from 'assert'
 import sinon from 'sinon'
 import globals from '../../shared/extensionGlobals'
-import { randomUUID } from '../../shared'
+import { randomUUID } from '../../shared/crypto'
+import * as setContext from '../../shared/vscode/setContext'
 import { assertTelemetry, installFakeClock } from '../testUtil'
 import {
     NotificationFetcher,
@@ -89,7 +90,7 @@ describe('Notifications Controller', function () {
     beforeEach(async function () {
         panelNode.setNotifications([], [])
         fetcher = new TestFetcher()
-        controller = new NotificationsController(panelNode, fetcher)
+        controller = new NotificationsController(panelNode, fetcher, '_aws.test.notification' as any)
 
         ruleEngineSpy = sinon.spy(ruleEngine, 'shouldDisplayNotification')
         focusPanelSpy = sinon.spy(panelNode, 'focusPanel')
@@ -107,6 +108,10 @@ describe('Notifications Controller', function () {
     })
 
     it('can fetch and store startup notifications', async function () {
+        // There seems to be a race condition with having a global spy object that is reset after each
+        // test. Doesn't seem to affect the other ones, for some reason.
+        const setContextSpy = sinon.spy(setContext, 'setContext')
+
         const eTag = randomUUID()
         const content = {
             schemaVersion: '1.x',
@@ -135,9 +140,14 @@ describe('Notifications Controller', function () {
         assert.deepStrictEqual(panelNode.startUpNotifications, [content.notifications[0]])
         assert.equal(panelNode.getChildren().length, 1)
         assert.equal(focusPanelSpy.callCount, 0)
+        assert.ok(setContextSpy.calledWithExactly('aws.toolkit.notifications.show', true))
+
+        setContextSpy.restore()
     })
 
     it('can fetch and store emergency notifications', async function () {
+        const setContextSpy = sinon.spy(setContext, 'setContext')
+
         const eTag = randomUUID()
         const content = {
             schemaVersion: '1.x',
@@ -166,9 +176,14 @@ describe('Notifications Controller', function () {
         assert.deepStrictEqual(panelNode.emergencyNotifications, [content.notifications[0]])
         assert.equal(panelNode.getChildren().length, 1)
         assert.equal(focusPanelSpy.callCount, 1)
+        assert.ok(setContextSpy.calledWithExactly('aws.toolkit.notifications.show', true))
+
+        setContextSpy.restore()
     })
 
     it('can fetch and store both startup and emergency notifications', async function () {
+        const setContextSpy = sinon.spy(setContext, 'setContext')
+
         const eTag1 = randomUUID()
         const eTag2 = randomUUID()
         const startUpContent = {
@@ -224,9 +239,14 @@ describe('Notifications Controller', function () {
         assert.deepStrictEqual(panelNode.emergencyNotifications, [emergencyContent.notifications[0]])
         assert.equal(panelNode.getChildren().length, 2)
         assert.equal(focusPanelSpy.callCount, 1)
+        assert.ok(setContextSpy.calledWithExactly('aws.toolkit.notifications.show', true))
+
+        setContextSpy.restore()
     })
 
     it('dismisses a startup notification', async function () {
+        const setContextSpy = sinon.spy(setContext, 'setContext')
+
         const eTag = randomUUID()
         const content = {
             schemaVersion: '1.x',
@@ -241,6 +261,7 @@ describe('Notifications Controller', function () {
 
         assert.equal(panelNode.getChildren().length, 2)
         assert.equal(panelNode.startUpNotifications.length, 2)
+        assert.ok(setContextSpy.calledWithExactly('aws.toolkit.notifications.show', true))
 
         assert.deepStrictEqual(await globals.globalState.get(controller.storageKey), {
             startUp: {
@@ -267,9 +288,13 @@ describe('Notifications Controller', function () {
 
         assert.equal(panelNode.getChildren().length, 1)
         assert.equal(panelNode.startUpNotifications.length, 1)
+
+        setContextSpy.restore()
     })
 
     it('does not redisplay dismissed notifications', async function () {
+        const setContextSpy = sinon.spy(setContext, 'setContext')
+
         const content = {
             schemaVersion: '1.x',
             notifications: [getValidTestNotification('id:startup1')],
@@ -281,9 +306,11 @@ describe('Notifications Controller', function () {
 
         await controller.pollForStartUp(ruleEngine)
         assert.equal(panelNode.getChildren().length, 1)
+        assert.deepStrictEqual(setContextSpy.lastCall.args, ['aws.toolkit.notifications.show', true])
 
         await dismissNotification(content.notifications[0])
         assert.equal(panelNode.getChildren().length, 0)
+        assert.deepStrictEqual(setContextSpy.lastCall.args, ['aws.toolkit.notifications.show', false])
 
         content.notifications.push(getValidTestNotification('id:startup2'))
         fetcher.setStartUpContent({
@@ -305,6 +332,9 @@ describe('Notifications Controller', function () {
         })
 
         assert.equal(panelNode.getChildren().length, 1)
+        assert.deepStrictEqual(setContextSpy.lastCall.args, ['aws.toolkit.notifications.show', true])
+
+        setContextSpy.restore()
     })
 
     it('does not refocus emergency notifications', async function () {
@@ -376,6 +406,8 @@ describe('Notifications Controller', function () {
     })
 
     it('cleans out dismissed state', async function () {
+        const setContextSpy = sinon.spy(setContext, 'setContext')
+
         const startUpContent = {
             schemaVersion: '1.x',
             notifications: [getValidTestNotification('id:startup1')],
@@ -434,6 +466,7 @@ describe('Notifications Controller', function () {
             newlyReceived: [],
         })
         assert.equal(panelNode.getChildren().length, 1)
+        assert.deepStrictEqual(setContextSpy.lastCall.args, ['aws.toolkit.notifications.show', true])
 
         fetcher.setEmergencyContent({
             eTag: '1',
@@ -455,6 +488,9 @@ describe('Notifications Controller', function () {
         })
 
         assert.equal(panelNode.getChildren().length, 0)
+        assert.deepStrictEqual(setContextSpy.lastCall.args, ['aws.toolkit.notifications.show', false])
+
+        setContextSpy.restore()
     })
 
     it('does not rethrow errors when fetching', async function () {

packages/core/src/test/notifications/rendering.test.ts

@@ -12,28 +12,20 @@ import { panelNode } from './controller.test'
 import { getTestWindow } from '../shared/vscode/window'
 import * as VsCodeUtils from '../../shared/utilities/vsCodeUtils'
 import { assertTextEditorContains, installFakeClock } from '../testUtil'
+import { waitUntil } from '../../shared/utilities/timeoutUtils'
+import globals from '../../shared/extensionGlobals'
 
 describe('Notifications Rendering', function () {
     let sandbox: sinon.SinonSandbox
-    let clock: FakeTimers.InstalledClock
-
-    before(function () {
-        clock = installFakeClock()
-    })
 
     beforeEach(function () {
         sandbox = sinon.createSandbox()
     })
 
     afterEach(function () {
-        clock.reset()
         sandbox.restore()
     })
 
-    after(function () {
-        clock.uninstall()
-    })
-
     // util to test txt pop-up under different senarios
     async function verifyTxtNotification(notification: ToolkitNotification) {
         const expectedContent = notification.uiRenderInstructions.content['en-US'].description
@@ -103,14 +95,21 @@ describe('Notifications Rendering', function () {
             message.selectItem('Update and Reload')
         })
         const excuteCommandStub = sandbox.stub(vscode.commands, 'executeCommand').resolves()
+        const telemetrySpy = sandbox.spy(globals.telemetry, 'flushRecords')
         const notification = getModalNotification()
-        await panelNode.openNotification(notification)
 
         // Update and Reload is put on a timer so that other methods (e.g. telemetry) can finish.
-        await clock.tickAsync(2000)
+        const clock: FakeTimers.InstalledClock = installFakeClock()
+        await panelNode.openNotification(notification)
+
+        await clock.tickAsync(1000)
+        clock.uninstall()
+
+        await waitUntil(async () => excuteCommandStub.called, { interval: 5, timeout: 5000 })
 
         assert.ok(excuteCommandStub.calledWith('workbench.extensions.installExtension', 'aws.toolkit.fake.extension'))
         assert.ok(excuteCommandStub.calledWith('workbench.action.reloadWindow'))
+        assert.ok(telemetrySpy.calledOnce)
     })
 
     it('executes openURL type button', async function () {