address some comments

Author: hayemaxi

packages/amazonq/package.json

@@ -411,7 +411,7 @@
         "commands": [
             {
                 "command": "_aws.amazonq.notifications.dismiss",
-                "title": "%AWS.notifications.dismiss.title%",
+                "title": "%AWS.generic.dismiss%",
                 "category": "%AWS.amazonq.title%",
                 "enablement": "isCloud9 || !aws.isWebExtHost",
                 "icon": "$(remove-close)"

packages/core/package.nls.json

@@ -6,7 +6,6 @@
     "AWS.amazonq.productName": "Amazon Q",
     "AWS.codecatalyst.submenu.title": "Manage CodeCatalyst",
     "AWS.notifications.title": "Notifications",
-    "AWS.notifications.dismiss.title": "Dismiss",
     "AWS.configuration.profileDescription": "The name of the credential profile to obtain credentials from.",
     "AWS.configuration.description.lambda.recentlyUploaded": "Recently selected Lambda upload targets.",
     "AWS.configuration.description.ecs.openTerminalCommand": "The command to run when starting a new interactive terminal session.",
@@ -246,6 +245,7 @@
     "AWS.generic.promptUpdate": "Update...",
     "AWS.generic.preview": "Preview",
     "AWS.generic.viewDocs": "View Documentation",
+    "AWS.generic.dismiss": "Dismiss",
     "AWS.ssmDocument.ssm.maxItemsComputed.desc": "Controls the maximum number of problems produced by the SSM Document language server.",
     "AWS.walkthrough.gettingStarted.title": "Get started with AWS",
     "AWS.walkthrough.gettingStarted.description": "These walkthroughs help you set up the AWS Toolkit.",

packages/core/src/notifications/controller.ts

@@ -7,7 +7,7 @@ import * as vscode from 'vscode'
 import { ToolkitError } from '../shared/errors'
 import globals from '../shared/extensionGlobals'
 import { globalKey } from '../shared/globalState'
-import { NotificationsState, NotificationType, NotificationData, ToolkitNotification } from './types'
+import { NotificationsState, NotificationsStateConstructor, NotificationType, ToolkitNotification } from './types'
 import { HttpResourceFetcher } from '../shared/resourcefetcher/httpResourceFetcher'
 import { getLogger } from '../shared/logger/logger'
 import { NotificationsNode } from './panelNode'
@@ -16,6 +16,7 @@ import { RuleEngine } from './rules'
 import { TreeNode } from '../shared/treeview/resourceTreeDataProvider'
 import { withRetries } from '../shared/utilities/functionUtils'
 import { FileResourceFetcher } from '../shared/resourcefetcher/fileResourceFetcher'
+import { isAmazonQ } from '../shared/extensionUtilities'
 
 const startUpEndpoint = 'https://idetoolkits-hostedfiles.amazonaws.com/Notifications/VSCode/startup/1.x.json'
 const emergencyEndpoint = 'https://idetoolkits-hostedfiles.amazonaws.com/Notifications/VSCode/emergency/1.x.json'
@@ -43,27 +44,21 @@ export class NotificationsController {
 
     /** Internal memory state that is written to global state upon modification. */
     private readonly state: NotificationsState
-    private readonly notificationsNode: NotificationsNode
 
     static #instance: NotificationsController | undefined
 
-    constructor(extPrefix: 'amazonq' | 'toolkit', node: NotificationsNode) {
+    constructor(private readonly notificationsNode: NotificationsNode) {
         if (!NotificationsController.#instance) {
-            registerDismissCommand(extPrefix)
+            registerDismissCommand()
         }
         NotificationsController.#instance = this
 
-        this.storageKey = `aws.${extPrefix}.notifications`
-        this.notificationsNode = node
-
-        this.state = globals.globalState.get(this.storageKey) ?? {
-            startUp: {} as NotificationData,
-            emergency: {} as NotificationData,
+        this.storageKey = 'aws.notifications'
+        this.state = globals.globalState.tryGet<NotificationsState>(this.storageKey, NotificationsStateConstructor, {
+            startUp: {},
+            emergency: {},
             dismissed: [],
-        }
-        this.state.startUp = this.state.startUp ?? {}
-        this.state.emergency = this.state.emergency ?? {}
-        this.state.dismissed = this.state.dismissed ?? []
+        })
     }
 
     public pollForStartUp(ruleEngine: RuleEngine) {
@@ -78,7 +73,7 @@ export class NotificationsController {
         try {
             await this.fetchNotifications(category)
         } catch (err: any) {
-            getLogger().error(`Unable to fetch %s notifications: %s`, category, err)
+            getLogger('notifications').error(`Unable to fetch %s notifications: %s`, category, err)
         }
 
         await this.displayNotifications(ruleEngine)
@@ -113,7 +108,7 @@ export class NotificationsController {
      * hide all notifications.
      */
     public async dismissNotification(notificationId: string) {
-        getLogger().debug('Dismissing notification: %s', notificationId)
+        getLogger('notifications').debug('Dismissing notification: %s', notificationId)
         this.state.dismissed.push(notificationId)
         await this.writeState()
 
@@ -126,17 +121,17 @@ export class NotificationsController {
     private async fetchNotifications(category: NotificationType) {
         const response = _useLocalFiles ? await this.fetchLocally(category) : await this.fetchRemotely(category)
         if (!response.content) {
-            getLogger().verbose('No new notifications for category: %s', category)
+            getLogger('notifications').verbose('No new notifications for category: %s', category)
             return
         }
 
-        getLogger().verbose('ETAG has changed for notifications category: %s', category)
+        getLogger('notifications').verbose('ETAG has changed for notifications category: %s', category)
 
         this.state[category].payload = JSON.parse(response.content)
         this.state[category].eTag = response.eTag
         await this.writeState()
 
-        getLogger().verbose(
+        getLogger('notifications').verbose(
             "Fetched notifications JSON for category '%s' with schema version: %s. There were %d notifications.",
             category,
             this.state[category].payload?.schemaVersion,
@@ -169,7 +164,7 @@ export class NotificationsController {
         const uri = category === 'startUp' ? startUpLocalPath : emergencyLocalPath
         const content = await new FileResourceFetcher(globals.context.asAbsolutePath(uri)).get()
 
-        getLogger().verbose('Fetched notifications locally for category: %s at path: %s', category, uri)
+        getLogger('notifications').verbose('Fetched notifications locally for category: %s at path: %s', category, uri)
         return {
             content,
             eTag: 'LOCAL_PATH',
@@ -180,7 +175,7 @@ export class NotificationsController {
      * Write the latest memory state to global state.
      */
     private async writeState() {
-        getLogger().debug('NotificationsController: Updating notifications state at %s', this.storageKey)
+        getLogger('notifications').debug('NotificationsController: Updating notifications state at %s', this.storageKey)
 
         // Clean out anything in 'dismissed' that doesn't exist anymore.
         const notifications = new Set(
@@ -203,8 +198,8 @@ export class NotificationsController {
     }
 }
 
-function registerDismissCommand(extPrefix: string) {
-    const name = `_aws.${extPrefix}.notifications.dismiss`
+function registerDismissCommand() {
+    const name = isAmazonQ() ? '_aws.amazonq.notifications.dismiss' : '_aws.toolkit.notifications.dismiss'
 
     globals.context.subscriptions.push(
         Commands.register(name, async (node: TreeNode) => {
@@ -216,7 +211,7 @@ function registerDismissCommand(extPrefix: string) {
 
                 await NotificationsController.instance.dismissNotification(notification.id)
             } else {
-                getLogger().error(`${name}: Cannot dismiss notification: item is not a vscode.TreeItem`)
+                getLogger('notifications').error(`${name}: Cannot dismiss notification: item is not a vscode.TreeItem`)
             }
         })
     )
@@ -229,5 +224,6 @@ function registerDismissCommand(extPrefix: string) {
 const _useLocalFiles = false
 export const _useLocalFilesCheck = _useLocalFiles // export for testing
 
+// Paths relative to current extension
 const startUpLocalPath = '../core/src/test/notifications/resources/startup/1.x.json'
 const emergencyLocalPath = '../core/src/test/notifications/resources/emergency/1.x.json'

packages/core/src/notifications/panelNode.ts

@@ -10,6 +10,7 @@ import { getIcon } from '../shared/icons'
 import { contextKey, setContext } from '../shared/vscode/setContext'
 import { NotificationType, ToolkitNotification } from './types'
 import { ToolkitError } from '../shared/errors'
+import { isAmazonQ } from '../shared/extensionUtilities'
 
 /**
  * Controls the "Notifications" side panel/tree in each extension. It takes purely UX actions
@@ -37,17 +38,25 @@ export class NotificationsNode implements TreeNode {
 
     static #instance: NotificationsNode
 
-    constructor(extPrefix: 'amazonq' | 'toolkit') {
+    constructor() {
         NotificationsNode.#instance = this
 
         this.openNotificationCmd = Commands.register(
-            `_aws.${extPrefix}.notifications.open`,
+            isAmazonQ() ? '_aws.amazonq.notifications.open' : '_aws.toolkit.notifications.open',
             async (n: ToolkitNotification) => this.openNotification(n)
         )
-        this.focusCmdStr = `aws.${extPrefix}.notifications.focus`
-        this.showContextStr = `aws.${extPrefix}.notifications.show`
-        this.startUpNodeContext = `${extPrefix}NotificationStartUp`
-        this.emergencyNodeContext = `${extPrefix}NotificationEmergency`
+
+        if (isAmazonQ()) {
+            this.focusCmdStr = 'aws.amazonq.notifications.focus'
+            this.showContextStr = 'aws.amazonq.notifications.show'
+            this.startUpNodeContext = 'amazonqNotificationStartUp'
+            this.emergencyNodeContext = 'amazonqNotificationEmergency'
+        } else {
+            this.focusCmdStr = 'aws.toolkit.notifications.focus'
+            this.showContextStr = 'aws.toolkit.notifications.show'
+            this.startUpNodeContext = 'toolkitNotificationStartUp'
+            this.emergencyNodeContext = 'toolkitNotificationEmergency'
+        }
     }
 
     public getTreeItem() {
@@ -73,7 +82,7 @@ export class NotificationsNode implements TreeNode {
 
     public getChildren() {
         const buildNode = (n: ToolkitNotification, type: NotificationType) => {
-            return this.openNotificationCmd!.build(n).asTreeNode({
+            return this.openNotificationCmd.build(n).asTreeNode({
                 label: n.uiRenderInstructions.content['en-US'].title,
                 iconPath: type === 'startUp' ? getIcon('vscode-question') : getIcon('vscode-alert'),
                 contextValue: type === 'startUp' ? this.startUpNodeContext : this.emergencyNodeContext,
@@ -103,7 +112,6 @@ export class NotificationsNode implements TreeNode {
      * Only dismisses startup notifications.
      */
     public dismissStartUpNotification(id: string) {
-        // Only startup notifications can be dismissed.
         this.startUpNotifications = this.startUpNotifications.filter((n) => n.id !== id)
         this.refresh()
     }
@@ -118,6 +126,8 @@ export class NotificationsNode implements TreeNode {
     /**
      * Fired when a notification is clicked on in the panel. It will run any rendering
      * instructions included in the notification. See {@link ToolkitNotification.uiRenderInstructions}.
+     *
+     * TODO: implement more rendering possibilites.
      */
     private async openNotification(notification: ToolkitNotification) {
         await vscode.window.showTextDocument(

packages/core/src/notifications/types.ts

@@ -5,6 +5,7 @@
 
 import * as vscode from 'vscode'
 import { EnvType, OperatingSystem } from '../shared/telemetry/util'
+import { TypeConstructor } from '../shared/utilities/typeConstructors'
 
 /** Types of information that we can use to determine whether to show a notification or not. */
 export type Criteria =
@@ -88,6 +89,23 @@ export type NotificationsState = {
     dismissed: string[]
 }
 
+export const NotificationsStateConstructor: TypeConstructor<NotificationsState> = (v: unknown): NotificationsState => {
+    if (v && typeof v === 'object' && isNotificationsState(v as Partial<NotificationsState>)) {
+        return v as NotificationsState
+    }
+    throw new Error('Cannot cast to NotificationsState.')
+}
+
+function isNotificationsState(v: Partial<NotificationsState>): v is NotificationsState {
+    const requiredKeys: (keyof NotificationsState)[] = ['startUp', 'emergency', 'dismissed']
+    return (
+        requiredKeys.every((key) => key in v) &&
+        Array.isArray(v.dismissed) &&
+        typeof v.startUp === 'object' &&
+        typeof v.emergency === 'object'
+    )
+}
+
 export type NotificationType = keyof Omit<NotificationsState, 'dismissed'>
 
 export interface RuleContext {

packages/core/src/shared/globalState.ts

@@ -32,7 +32,7 @@ export type globalKey =
     | 'aws.amazonq.hasShownWalkthrough'
     | 'aws.amazonq.showTryChatCodeLens'
     | 'aws.amazonq.notifications'
-    | 'aws.toolkit.notifications'
+    | 'aws.notifications'
     | 'aws.downloadPath'
     | 'aws.lastTouchedS3Folder'
     | 'aws.lastUploadedToS3Folder'

packages/core/src/shared/logger/toolkitLogger.ts

@@ -15,7 +15,7 @@ import { ConsoleLogTransport } from './consoleLogTransport'
 import { isWeb } from '../extensionGlobals'
 
 /* define log topics */
-export type LogTopic = 'unknown' | 'test' | 'crashReport'
+export type LogTopic = 'unknown' | 'test' | 'crashReport' | 'notifications'
 
 class ErrorLog {
     constructor(

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

@@ -16,7 +16,7 @@ import { installFakeClock } from '../testUtil'
 import * as FakeTimers from '@sinonjs/fake-timers'
 
 describe('Notifications Controller', function () {
-    const panelNode: NotificationsNode = new NotificationsNode('toolkit')
+    const panelNode: NotificationsNode = new NotificationsNode()
     let controller: NotificationsController
     const ruleEngine: RuleEngine = new RuleEngine({
         ideVersion: '1.83.0',
@@ -60,7 +60,7 @@ describe('Notifications Controller', function () {
 
     beforeEach(async function () {
         panelNode.setNotifications([], [])
-        controller = new NotificationsController('toolkit', panelNode)
+        controller = new NotificationsController(panelNode)
 
         sandbox = sinon.createSandbox()
 

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

@@ -0,0 +1,52 @@
+/*!
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import assert from 'assert'
+import { NotificationsState, NotificationsStateConstructor } from '../../notifications/types'
+
+describe('NotificationsState type validation', function () {
+    it('passes on valid input', async function () {
+        const state: NotificationsState = {
+            startUp: {},
+            emergency: {},
+            dismissed: [],
+        }
+        let ret
+        assert.doesNotThrow(() => {
+            ret = NotificationsStateConstructor(state)
+        })
+        assert.deepStrictEqual(ret, state)
+    })
+
+    it('fails on invalid input', async function () {
+        assert.throws(() => {
+            NotificationsStateConstructor('' as unknown as NotificationsState)
+        })
+        assert.throws(() => {
+            NotificationsStateConstructor({} as NotificationsState)
+        })
+        assert.throws(() => {
+            NotificationsStateConstructor({
+                startUp: {},
+                emergency: {},
+                dismissed: {}, // x
+            } as NotificationsState)
+        })
+        assert.throws(() => {
+            NotificationsStateConstructor({
+                startUp: {},
+                emergency: [], // x
+                dismissed: [],
+            } as NotificationsState)
+        })
+        assert.throws(() => {
+            NotificationsStateConstructor({
+                startUp: '', // x
+                emergency: {},
+                dismissed: [],
+            } as NotificationsState)
+        })
+    })
+})

packages/toolkit/package.json

@@ -2053,7 +2053,7 @@
         "commands": [
             {
                 "command": "_aws.toolkit.notifications.dismiss",
-                "title": "%AWS.notifications.dismiss.title%",
+                "title": "%AWS.generic.dismiss%",
                 "category": "%AWS.title%",
                 "enablement": "isCloud9 || !aws.isWebExtHost",
                 "icon": "$(remove-close)"