From cf46d56d9f5b3727751a841530c2c477a732f90e Mon Sep 17 00:00:00 2001 From: Maxim Hayes Date: Mon, 18 Nov 2024 11:50:06 -0500 Subject: [PATCH] fix(notifications): update types, field names, cross-window syncing - Make types more granular. - Align names - fix onRecieve typo, openTxt -> openTextDocument - Sync from global state on reload to detect activity in other windows. This makes it so that if users dismiss in one window, it will dismiss in all windows. It will also prevent a bug where users may get dismissed notifications again if they have multiple windows. --- packages/core/src/auth/auth.ts | 2 - packages/core/src/auth/connection.ts | 6 ++ packages/core/src/auth/sso/validation.ts | 4 +- .../util/supplementalContext/rankBm25.ts | 2 +- packages/core/src/notifications/activation.ts | 3 +- packages/core/src/notifications/controller.ts | 40 +++++++++--- packages/core/src/notifications/panelNode.ts | 9 +-- packages/core/src/notifications/rules.ts | 4 +- packages/core/src/notifications/types.ts | 62 +++++++++++++++---- .../src/test/awsService/ec2/model.test.ts | 2 +- .../src/test/notifications/controller.test.ts | 4 +- .../src/test/notifications/rendering.test.ts | 12 ++-- .../resources/emergency/1.x.json | 4 +- .../notifications/resources/startup/1.x.json | 4 +- .../core/src/test/notifications/rules.test.ts | 10 +-- 15 files changed, 119 insertions(+), 49 deletions(-) diff --git a/packages/core/src/auth/auth.ts b/packages/core/src/auth/auth.ts index 334c3841216..3335f8ab333 100644 --- a/packages/core/src/auth/auth.ts +++ b/packages/core/src/auth/auth.ts @@ -126,8 +126,6 @@ export interface ConnectionStateChangeEvent { readonly state: ProfileMetadata['connectionState'] } -export type AuthType = Auth - export type DeletedConnection = { connId: Connection['id']; storedProfile?: StoredProfile } type DeclaredConnection = Pick & { source: string } diff --git a/packages/core/src/auth/connection.ts b/packages/core/src/auth/connection.ts index 5fc505a0b5f..3e7752dd8e9 100644 --- a/packages/core/src/auth/connection.ts +++ b/packages/core/src/auth/connection.ts @@ -24,6 +24,7 @@ const warnOnce = onceChanged((s: string, url: string) => { void showMessageWithUrl(s, url, undefined, 'warn') }) +// TODO: Refactor all scopes to a central file with minimal dependencies. export const scopesCodeCatalyst = ['codecatalyst:read_write'] export const scopesSsoAccountAccess = ['sso:account:access'] /** These are the non-chat scopes for CW. */ @@ -39,6 +40,11 @@ type SsoType = | 'idc' // AWS Identity Center | 'builderId' +// TODO: This type is not centralized and there are many routines in the codebase that use some +// variation for these for validation, telemetry, UX, etc. A refactor is needed to align these +// string types. +export type AuthType = 'credentials' | 'builderId' | 'identityCenter' | 'unknown' + export const isIamConnection = (conn?: Connection): conn is IamConnection => conn?.type === 'iam' export const isSsoConnection = (conn?: Connection, type: SsoType = 'any'): conn is SsoConnection => { if (conn?.type !== 'sso') { diff --git a/packages/core/src/auth/sso/validation.ts b/packages/core/src/auth/sso/validation.ts index ecfe935a837..cad6aa7f4c3 100644 --- a/packages/core/src/auth/sso/validation.ts +++ b/packages/core/src/auth/sso/validation.ts @@ -4,7 +4,7 @@ */ import * as vscode from 'vscode' import { UnknownError } from '../../shared/errors' -import { AuthType } from '../auth' +import { Auth } from '../auth' import { SsoConnection, hasScopes, isAnySsoConnection } from '../connection' import { ssoUrlFormatMessage, ssoUrlFormatRegex } from './constants' @@ -19,7 +19,7 @@ export function validateSsoUrlFormat(url: string) { } export async function validateIsNewSsoUrlAsync( - auth: AuthType, + auth: Auth, url: string, requiredScopes?: string[] ): Promise { diff --git a/packages/core/src/codewhisperer/util/supplementalContext/rankBm25.ts b/packages/core/src/codewhisperer/util/supplementalContext/rankBm25.ts index a6fd84aafd6..3e00013c9cf 100644 --- a/packages/core/src/codewhisperer/util/supplementalContext/rankBm25.ts +++ b/packages/core/src/codewhisperer/util/supplementalContext/rankBm25.ts @@ -7,7 +7,7 @@ export interface BM25Document { content: string - /** The score that the document recieves. */ + /** The score that the document receives. */ score: number index: number diff --git a/packages/core/src/notifications/activation.ts b/packages/core/src/notifications/activation.ts index 2ac36bcd5bd..cd732909d16 100644 --- a/packages/core/src/notifications/activation.ts +++ b/packages/core/src/notifications/activation.ts @@ -11,9 +11,10 @@ import { RuleEngine, getRuleContext } from './rules' import globals from '../shared/extensionGlobals' import { AuthState } from './types' import { getLogger } from '../shared/logger/logger' +import { oneMinute } from '../shared/datetime' /** Time in MS to poll for emergency notifications */ -const emergencyPollTime = 1000 * 10 * 60 +const emergencyPollTime = oneMinute * 10 /** * Activate the in-IDE notifications module and begin receiving notifications. diff --git a/packages/core/src/notifications/controller.ts b/packages/core/src/notifications/controller.ts index a93835d89b1..30aa8ef3570 100644 --- a/packages/core/src/notifications/controller.ts +++ b/packages/core/src/notifications/controller.ts @@ -9,6 +9,7 @@ import globals from '../shared/extensionGlobals' import { globalKey } from '../shared/globalState' import { getNotificationTelemetryId, + Notifications, NotificationsState, NotificationsStateConstructor, NotificationType, @@ -56,12 +57,7 @@ export class NotificationsController { } NotificationsController.#instance = this - this.state = globals.globalState.tryGet(this.storageKey, NotificationsStateConstructor, { - startUp: {}, - emergency: {}, - dismissed: [], - newlyReceived: [], - }) + this.state = this.getDefaultState() } public pollForStartUp(ruleEngine: RuleEngine) { @@ -74,6 +70,9 @@ export class NotificationsController { private async poll(ruleEngine: RuleEngine, category: NotificationType) { try { + // Get latest state in case it was modified by other windows. + // It is a minimal read to avoid race conditions. + this.readState() await this.fetchNotifications(category) } catch (err: any) { getLogger('notifications').error(`Unable to fetch %s notifications: %s`, category, err) @@ -139,7 +138,7 @@ export class NotificationsController { return } // Parse the notifications - const newPayload = JSON.parse(response.content) + const newPayload: Notifications = JSON.parse(response.content) const newNotifications = newPayload.notifications ?? [] // Get the current notifications @@ -188,6 +187,33 @@ export class NotificationsController { await globals.globalState.update(this.storageKey, this.state) } + /** + * Read relevant values from the latest global state to memory. Useful to bring changes from other windows. + * + * Currently only brings dismissed, so users with multiple vscode instances open do not have issues with + * dismissing notifications multiple times. Otherwise, each instance has an independent session for + * displaying the notifications (e.g. multiple windows can be blocked in critical emergencies). + * + * Note: This sort of pattern (reading back and forth from global state in async functions) is prone to + * race conditions, which is why we limit the read to the fairly inconsequential `dismissed` property. + */ + private readState() { + const state = this.getDefaultState() + this.state.dismissed = state.dismissed + } + + /** + * Returns stored notification state, or a default state object if it is invalid or undefined. + */ + private getDefaultState() { + return globals.globalState.tryGet(this.storageKey, NotificationsStateConstructor, { + startUp: {}, + emergency: {}, + dismissed: [], + newlyReceived: [], + }) + } + static get instance() { if (this.#instance === undefined) { throw new ToolkitError('NotificationsController was accessed before it has been initialized.') diff --git a/packages/core/src/notifications/panelNode.ts b/packages/core/src/notifications/panelNode.ts index 94e4f2031f1..fe2512be47b 100644 --- a/packages/core/src/notifications/panelNode.ts +++ b/packages/core/src/notifications/panelNode.ts @@ -159,7 +159,8 @@ export class NotificationsNode implements TreeNode { * instructions included in the notification. See {@link ToolkitNotification.uiRenderInstructions}. */ public async openNotification(notification: ToolkitNotification) { - switch (notification.uiRenderInstructions.onClick.type) { + const onClickType = notification.uiRenderInstructions.onClick.type + switch (onClickType) { case 'modal': // Render blocking modal logger.verbose(`rendering modal for notificaiton: ${notification.id} ...`) @@ -180,7 +181,7 @@ export class NotificationsNode implements TreeNode { // Display read-only txt document logger.verbose(`showing txt document for notification: ${notification.id} ...`) await telemetry.toolkit_invokeAction.run(async () => { - telemetry.record({ source: getNotificationTelemetryId(notification), action: 'openTxt' }) + telemetry.record({ source: getNotificationTelemetryId(notification), action: onClickType }) await readonlyDocument.show( notification.uiRenderInstructions.content['en-US'].description, `Notification: ${notification.id}` @@ -230,7 +231,7 @@ export class NotificationsNode implements TreeNode { // Different button options if (selectedButton) { switch (selectedButton.type) { - case 'openTxt': + case 'openTextDocument': await readonlyDocument.show( notification.uiRenderInstructions.content['en-US'].description, `Notification: ${notification.id}` @@ -260,7 +261,7 @@ export class NotificationsNode implements TreeNode { public async onReceiveNotifications(notifications: ToolkitNotification[]) { for (const notification of notifications) { - void this.showInformationWindow(notification, notification.uiRenderInstructions.onRecieve, true) + void this.showInformationWindow(notification, notification.uiRenderInstructions.onReceive, true) } } diff --git a/packages/core/src/notifications/rules.ts b/packages/core/src/notifications/rules.ts index 9e62adb7cc1..34f5660a50e 100644 --- a/packages/core/src/notifications/rules.ts +++ b/packages/core/src/notifications/rules.ts @@ -10,6 +10,7 @@ import { ConditionalClause, RuleContext, DisplayIf, CriteriaCondition, ToolkitNo import { getComputeEnvType, getOperatingSystem } from '../shared/telemetry/util' import { AuthFormId } from '../login/webview/vue/types' import { getLogger } from '../shared/logger/logger' +import { ToolkitError } from '../shared/errors' const logger = getLogger('notifications') /** @@ -158,7 +159,8 @@ export class RuleEngine { case 'ActiveExtensions': return isSuperSetOfExpected(this.context.activeExtensions) default: - throw new Error(`Unknown criteria type: ${criteria.type}`) + logger.error('Unknown criteria passed to RuleEngine: %O', criteria) + throw new ToolkitError(`Unknown criteria type: ${(criteria as any).type}`) } } } diff --git a/packages/core/src/notifications/types.ts b/packages/core/src/notifications/types.ts index 59717cf9079..5ee06df5cd9 100644 --- a/packages/core/src/notifications/types.ts +++ b/packages/core/src/notifications/types.ts @@ -7,16 +7,55 @@ import * as vscode from 'vscode' import { EnvType, OperatingSystem } from '../shared/telemetry/util' import { TypeConstructor } from '../shared/utilities/typeConstructors' import { AuthUserState, AuthStatus } from '../shared/telemetry/telemetry.gen' +import { AuthType } from '../auth/connection' /** Types of information that we can use to determine whether to show a notification or not. */ -export type Criteria = 'OS' | 'ComputeEnv' | 'AuthType' | 'AuthRegion' | 'AuthState' | 'AuthScopes' | 'ActiveExtensions' -/** Generic condition where the type determines how the values are evaluated. */ -export interface CriteriaCondition { - readonly type: Criteria - readonly values: string[] +type OsCriteria = { + type: 'OS' + values: OperatingSystem[] +} + +type ComputeEnvCriteria = { + type: 'ComputeEnv' + values: EnvType[] +} + +type AuthTypeCriteria = { + type: 'AuthType' + values: AuthType[] +} + +type AuthRegionCriteria = { + type: 'AuthRegion' + values: string[] +} + +type AuthStateCriteria = { + type: 'AuthState' + values: AuthStatus[] +} + +type AuthScopesCriteria = { + type: 'AuthScopes' + values: string[] // TODO: Scopes should be typed. Could import here, but don't want to import too much. +} + +type ActiveExtensionsCriteria = { + type: 'ActiveExtensions' + values: string[] } +/** Generic condition where the type determines how the values are evaluated. */ +export type CriteriaCondition = + | OsCriteria + | ComputeEnvCriteria + | AuthTypeCriteria + | AuthRegionCriteria + | AuthStateCriteria + | AuthScopesCriteria + | ActiveExtensionsCriteria + /** One of the subconditions (clauses) must match to be valid. */ export interface OR { readonly type: 'or' @@ -39,8 +78,8 @@ export interface ExactMatch { export type ConditionalClause = Range | ExactMatch | OR export type OnReceiveType = 'toast' | 'modal' -export type OnClickType = 'modal' | 'openTextDocument' | 'openUrl' -export type ActionType = 'openUrl' | 'updateAndReload' | 'openTxt' +export type OnClickType = { type: 'modal' } | { type: 'openTextDocument' } | { type: 'openUrl'; url: string } +export type ActionType = 'openUrl' | 'updateAndReload' | 'openTextDocument' /** How to display the notification. */ export interface UIRenderInstructions { @@ -51,11 +90,8 @@ export interface UIRenderInstructions { toastPreview?: string // optional property for toast } } - onRecieve: OnReceiveType // TODO: typo - onClick: { - type: OnClickType - url?: string // optional property for 'openUrl' - } + onReceive: OnReceiveType + onClick: OnClickType actions?: Array<{ type: ActionType displayText: { @@ -124,7 +160,7 @@ export interface RuleContext { readonly extensionVersion: string readonly os: OperatingSystem readonly computeEnv: EnvType - readonly authTypes: ('credentials' | 'builderId' | 'identityCenter' | 'unknown')[] + readonly authTypes: AuthType[] readonly authRegions: string[] readonly authStates: AuthStatus[] readonly authScopes: string[] diff --git a/packages/core/src/test/awsService/ec2/model.test.ts b/packages/core/src/test/awsService/ec2/model.test.ts index 6c7f6f1fb4a..d39de4ba1da 100644 --- a/packages/core/src/test/awsService/ec2/model.test.ts +++ b/packages/core/src/test/awsService/ec2/model.test.ts @@ -24,7 +24,7 @@ describe('Ec2ConnectClient', function () { }) describe('getAttachedIamRole', async function () { - it('only returns role if recieves ARN from instance profile', async function () { + it('only returns role if receives ARN from instance profile', async function () { let role: IAM.Role | undefined const getInstanceProfileStub = sinon.stub(Ec2Client.prototype, 'getAttachedIamInstanceProfile') diff --git a/packages/core/src/test/notifications/controller.test.ts b/packages/core/src/test/notifications/controller.test.ts index c1e2d299463..25b013db71b 100644 --- a/packages/core/src/test/notifications/controller.test.ts +++ b/packages/core/src/test/notifications/controller.test.ts @@ -557,7 +557,7 @@ function getValidTestNotification(id: string): ToolkitNotification { description: 'test', }, }, - onRecieve: 'toast', + onReceive: 'toast', onClick: { type: 'openUrl', url: 'https://aws.amazon.com/visualstudiocode/', @@ -580,7 +580,7 @@ function getInvalidTestNotification(id: string): ToolkitNotification { description: 'test', }, }, - onRecieve: 'toast', + onReceive: 'toast', onClick: { type: 'modal' }, }, } diff --git a/packages/core/src/test/notifications/rendering.test.ts b/packages/core/src/test/notifications/rendering.test.ts index e446e53fdba..ccda76195a2 100644 --- a/packages/core/src/test/notifications/rendering.test.ts +++ b/packages/core/src/test/notifications/rendering.test.ts @@ -112,7 +112,7 @@ describe('Notifications Rendering', function () { assert.ok(telemetrySpy.calledOnce) }) - it('executes openURL type button', async function () { + it('executes openUrl type button', async function () { const testWindow = getTestWindow() testWindow.onDidShowMessage((message) => { // Simulate user clicking open URL type @@ -122,7 +122,7 @@ describe('Notifications Rendering', function () { await verifyOpenExternalUrl(notification) }) - it('executes openTxt type button', async function () { + it('executes openTextDocument type button', async function () { const testWindow = getTestWindow() testWindow.onDidShowMessage((message) => { // Simulate user clicking open txt type @@ -148,7 +148,7 @@ function getToastURLTestNotification(): ToolkitNotification { toastPreview: 'test toast preview', }, }, - onRecieve: 'toast', + onReceive: 'toast', onClick: { type: 'openUrl', url: 'https://aws.amazon.com/visualstudiocode/', @@ -170,7 +170,7 @@ function getTxtNotification(): ToolkitNotification { description: 'This is a text document notification.', }, }, - onRecieve: 'toast', + onReceive: 'toast', onClick: { type: 'openTextDocument', }, @@ -191,7 +191,7 @@ function getModalNotification(): ToolkitNotification { description: 'This is a modal notification.', }, }, - onRecieve: 'modal', + onReceive: 'modal', onClick: { type: 'modal', }, @@ -210,7 +210,7 @@ function getModalNotification(): ToolkitNotification { }, }, { - type: 'openTxt', + type: 'openTextDocument', displayText: { 'en-US': 'Read More', }, diff --git a/packages/core/src/test/notifications/resources/emergency/1.x.json b/packages/core/src/test/notifications/resources/emergency/1.x.json index ced10965ab5..b0a83f40d3b 100644 --- a/packages/core/src/test/notifications/resources/emergency/1.x.json +++ b/packages/core/src/test/notifications/resources/emergency/1.x.json @@ -14,7 +14,7 @@ "toastPreview": "Signing into Amazon Q is broken, please try this workaround while we work on releasing a fix." } }, - "onRecieve": "toast", + "onReceive": "toast", "onClick": { "type": "openTextDocument" }, @@ -34,7 +34,7 @@ "toastPreview": "This version of Amazon Q is currently broken, please update to avoid issues." } }, - "onRecieve": "toast", + "onReceive": "toast", "onClick": { "type": "modal" }, diff --git a/packages/core/src/test/notifications/resources/startup/1.x.json b/packages/core/src/test/notifications/resources/startup/1.x.json index 29a7e7e89cd..aae5b1d70f2 100644 --- a/packages/core/src/test/notifications/resources/startup/1.x.json +++ b/packages/core/src/test/notifications/resources/startup/1.x.json @@ -14,7 +14,7 @@ "toastPreview": "New Amazon Q features available: inline chat" } }, - "onRecieve": "toast", + "onReceive": "toast", "onClick": { "type": "openTextDocument" }, @@ -41,7 +41,7 @@ "toastPreview": "New Amazon Q features are available!" } }, - "onRecieve": "toast", + "onReceive": "toast", "onClick": { "type": "openUrl", "url": "https://aws.amazon.com/developer/generative-ai/amazon-q/change-log/" diff --git a/packages/core/src/test/notifications/rules.test.ts b/packages/core/src/test/notifications/rules.test.ts index 867b2119546..0df695c1be6 100644 --- a/packages/core/src/test/notifications/rules.test.ts +++ b/packages/core/src/test/notifications/rules.test.ts @@ -40,7 +40,7 @@ describe('Notifications Rule Engine', function () { description: 'Something crazy is happening! Please update your extension.', }, }, - onRecieve: 'toast', + onReceive: 'toast', onClick: { type: 'openUrl', url: 'https://aws.amazon.com/visualstudiocode/', @@ -305,7 +305,7 @@ describe('Notifications Rule Engine', function () { assert.equal( ruleEngine.shouldDisplayNotification( buildNotification({ - additionalCriteria: [{ type: 'AuthType', values: ['builderId', 'iamIdentityCenter'] }], + additionalCriteria: [{ type: 'AuthType', values: ['builderId', 'identityCenter'] }], }) ), true @@ -316,7 +316,7 @@ describe('Notifications Rule Engine', function () { assert.equal( ruleEngine.shouldDisplayNotification( buildNotification({ - additionalCriteria: [{ type: 'AuthType', values: ['iamIdentityCenter'] }], + additionalCriteria: [{ type: 'AuthType', values: ['identityCenter'] }], }) ), false @@ -452,7 +452,7 @@ describe('Notifications Rule Engine', function () { additionalCriteria: [ { type: 'OS', values: ['LINUX', 'MAC'] }, { type: 'ComputeEnv', values: ['local', 'ec2'] }, - { type: 'AuthType', values: ['builderId', 'iamIdentityCenter'] }, + { type: 'AuthType', values: ['builderId', 'identityCenter'] }, { type: 'AuthRegion', values: ['us-east-1', 'us-west-2'] }, { type: 'AuthState', values: ['connected'] }, { type: 'AuthScopes', values: ['codewhisperer:completions', 'codewhisperer:analysis'] }, @@ -489,7 +489,7 @@ describe('Notifications Rule Engine', function () { }, additionalCriteria: [ { type: 'OS', values: ['LINUX', 'MAC'] }, - { type: 'AuthType', values: ['builderId', 'iamIdentityCenter'] }, + { type: 'AuthType', values: ['builderId', 'identityCenter'] }, { type: 'AuthRegion', values: ['us-east-1', 'us-west-2'] }, { type: 'AuthState', values: ['connected'] }, { type: 'AuthScopes', values: ['codewhisperer:completions', 'codewhisperer:analysis'] },