Skip to content

Commit 360c7f6

Browse files
hayemaxihayemaxi
committed
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.
1 parent 135c3e5 commit 360c7f6

File tree

12 files changed

+111
-45
lines changed

12 files changed

+111
-45
lines changed

‎packages/core/src/codewhisperer/util/supplementalContext/rankBm25.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77

88
export interface BM25Document {
99
content: string
10-
/** The score that the document recieves. */
10+
/** The score that the document receives. */
1111
score: number
1212

1313
index: number

‎packages/core/src/notifications/activation.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import { AuthState } from './types'
1313
import { getLogger } from '../shared/logger/logger'
1414

1515
/** Time in MS to poll for emergency notifications */
16-
const emergencyPollTime = 1000 * 10 * 60
16+
const emergencyPollTime = 1000 * 60 * 10
1717

1818
/**
1919
* Activate the in-IDE notifications module and begin receiving notifications.

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

Lines changed: 33 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import globals from '../shared/extensionGlobals'
99
import { globalKey } from '../shared/globalState'
1010
import {
1111
getNotificationTelemetryId,
12+
Notifications,
1213
NotificationsState,
1314
NotificationsStateConstructor,
1415
NotificationType,
@@ -56,12 +57,7 @@ export class NotificationsController {
5657
}
5758
NotificationsController.#instance = this
5859

59-
this.state = globals.globalState.tryGet<NotificationsState>(this.storageKey, NotificationsStateConstructor, {
60-
startUp: {},
61-
emergency: {},
62-
dismissed: [],
63-
newlyReceived: [],
64-
})
60+
this.state = this.getDefaultState()
6561
}
6662

6763
public pollForStartUp(ruleEngine: RuleEngine) {
@@ -74,6 +70,9 @@ export class NotificationsController {
7470

7571
private async poll(ruleEngine: RuleEngine, category: NotificationType) {
7672
try {
73+
// Get latest state in case it was modified by other windows.
74+
// It is a minimal read to avoid race conditions.
75+
this.readState()
7776
await this.fetchNotifications(category)
7877
} catch (err: any) {
7978
getLogger('notifications').error(`Unable to fetch %s notifications: %s`, category, err)
@@ -139,7 +138,7 @@ export class NotificationsController {
139138
return
140139
}
141140
// Parse the notifications
142-
const newPayload = JSON.parse(response.content)
141+
const newPayload: Notifications = JSON.parse(response.content)
143142
const newNotifications = newPayload.notifications ?? []
144143

145144
// Get the current notifications
@@ -188,6 +187,33 @@ export class NotificationsController {
188187
await globals.globalState.update(this.storageKey, this.state)
189188
}
190189

190+
/**
191+
* Read relevant values from the latest global state to memory. Useful to bring changes from other windows.
192+
*
193+
* Currently only brings dismissed, so users with multiple vscode instances open do not have issues with
194+
* dismissing notifications multiple times. Otherwise, each instance has an independent session for
195+
* displaying the notifications (e.g. multiple windows can be blocked in critical emergencies).
196+
*
197+
* Note: This sort of pattern (reading back and forth from global state in async functions) is prone to
198+
* race conditions, which is why we limit the read to the fairly inconsequential `dismissed` property.
199+
*/
200+
private readState() {
201+
const state = this.getDefaultState()
202+
this.state.dismissed = state.dismissed
203+
}
204+
205+
/**
206+
* Returns stored notification state, or a default state object if it is invalid or undefined.
207+
*/
208+
private getDefaultState() {
209+
return globals.globalState.tryGet<NotificationsState>(this.storageKey, NotificationsStateConstructor, {
210+
startUp: {},
211+
emergency: {},
212+
dismissed: [],
213+
newlyReceived: [],
214+
})
215+
}
216+
191217
static get instance() {
192218
if (this.#instance === undefined) {
193219
throw new ToolkitError('NotificationsController was accessed before it has been initialized.')

‎packages/core/src/notifications/panelNode.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,8 @@ export class NotificationsNode implements TreeNode {
159159
* instructions included in the notification. See {@link ToolkitNotification.uiRenderInstructions}.
160160
*/
161161
public async openNotification(notification: ToolkitNotification) {
162-
switch (notification.uiRenderInstructions.onClick.type) {
162+
const onClickType = notification.uiRenderInstructions.onClick.type
163+
switch (onClickType) {
163164
case 'modal':
164165
// Render blocking modal
165166
logger.verbose(`rendering modal for notificaiton: ${notification.id} ...`)
@@ -180,7 +181,7 @@ export class NotificationsNode implements TreeNode {
180181
// Display read-only txt document
181182
logger.verbose(`showing txt document for notification: ${notification.id} ...`)
182183
await telemetry.toolkit_invokeAction.run(async () => {
183-
telemetry.record({ source: getNotificationTelemetryId(notification), action: 'openTxt' })
184+
telemetry.record({ source: getNotificationTelemetryId(notification), action: onClickType })
184185
await readonlyDocument.show(
185186
notification.uiRenderInstructions.content['en-US'].description,
186187
`Notification: ${notification.id}`
@@ -230,7 +231,7 @@ export class NotificationsNode implements TreeNode {
230231
// Different button options
231232
if (selectedButton) {
232233
switch (selectedButton.type) {
233-
case 'openTxt':
234+
case 'openTextDocument':
234235
await readonlyDocument.show(
235236
notification.uiRenderInstructions.content['en-US'].description,
236237
`Notification: ${notification.id}`
@@ -260,7 +261,7 @@ export class NotificationsNode implements TreeNode {
260261

261262
public async onReceiveNotifications(notifications: ToolkitNotification[]) {
262263
for (const notification of notifications) {
263-
void this.showInformationWindow(notification, notification.uiRenderInstructions.onRecieve, true)
264+
void this.showInformationWindow(notification, notification.uiRenderInstructions.onReceive, true)
264265
}
265266
}
266267

‎packages/core/src/notifications/rules.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { ConditionalClause, RuleContext, DisplayIf, CriteriaCondition, ToolkitNo
1010
import { getComputeEnvType, getOperatingSystem } from '../shared/telemetry/util'
1111
import { AuthFormId } from '../login/webview/vue/types'
1212
import { getLogger } from '../shared/logger/logger'
13+
import { ToolkitError } from '../shared/errors'
1314

1415
const logger = getLogger('notifications')
1516
/**
@@ -158,7 +159,8 @@ export class RuleEngine {
158159
case 'ActiveExtensions':
159160
return isSuperSetOfExpected(this.context.activeExtensions)
160161
default:
161-
throw new Error(`Unknown criteria type: ${criteria.type}`)
162+
logger.error('Unknown criteria passed to RuleEngine: %O', criteria)
163+
throw new ToolkitError(`Unknown criteria type: ${(criteria as any).type}`)
162164
}
163165
}
164166
}

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

Lines changed: 50 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,52 @@ import { TypeConstructor } from '../shared/utilities/typeConstructors'
99
import { AuthUserState, AuthStatus } from '../shared/telemetry/telemetry.gen'
1010

1111
/** Types of information that we can use to determine whether to show a notification or not. */
12-
export type Criteria = 'OS' | 'ComputeEnv' | 'AuthType' | 'AuthRegion' | 'AuthState' | 'AuthScopes' | 'ActiveExtensions'
1312

14-
/** Generic condition where the type determines how the values are evaluated. */
15-
export interface CriteriaCondition {
16-
readonly type: Criteria
17-
readonly values: string[]
13+
type OsCriteria = {
14+
type: 'OS'
15+
values: OperatingSystem[]
16+
}
17+
18+
type ComputeEnvCriteria = {
19+
type: 'ComputeEnv'
20+
values: EnvType[]
21+
}
22+
23+
type AuthTypeCriteria = {
24+
type: 'AuthType'
25+
values: AuthType[]
26+
}
27+
28+
type AuthRegionCriteria = {
29+
type: 'AuthRegion'
30+
values: string[]
31+
}
32+
33+
type AuthStateCriteria = {
34+
type: 'AuthState'
35+
values: AuthStatus[]
1836
}
1937

38+
type AuthScopesCriteria = {
39+
type: 'AuthScopes'
40+
values: string[] // TODO: Scopes should be typed. Could import here, but don't want to import too much.
41+
}
42+
43+
type ActiveExtensionsCriteria = {
44+
type: 'ActiveExtensions'
45+
values: string[]
46+
}
47+
48+
/** Generic condition where the type determines how the values are evaluated. */
49+
export type CriteriaCondition =
50+
| OsCriteria
51+
| ComputeEnvCriteria
52+
| AuthTypeCriteria
53+
| AuthRegionCriteria
54+
| AuthStateCriteria
55+
| AuthScopesCriteria
56+
| ActiveExtensionsCriteria
57+
2058
/** One of the subconditions (clauses) must match to be valid. */
2159
export interface OR {
2260
readonly type: 'or'
@@ -39,8 +77,8 @@ export interface ExactMatch {
3977
export type ConditionalClause = Range | ExactMatch | OR
4078

4179
export type OnReceiveType = 'toast' | 'modal'
42-
export type OnClickType = 'modal' | 'openTextDocument' | 'openUrl'
43-
export type ActionType = 'openUrl' | 'updateAndReload' | 'openTxt'
80+
export type OnClickType = { type: 'modal' } | { type: 'openTextDocument' } | { type: 'openUrl'; url: string }
81+
export type ActionType = 'openUrl' | 'updateAndReload' | 'openTextDocument'
4482

4583
/** How to display the notification. */
4684
export interface UIRenderInstructions {
@@ -51,11 +89,8 @@ export interface UIRenderInstructions {
5189
toastPreview?: string // optional property for toast
5290
}
5391
}
54-
onRecieve: OnReceiveType // TODO: typo
55-
onClick: {
56-
type: OnClickType
57-
url?: string // optional property for 'openUrl'
58-
}
92+
onReceive: OnReceiveType
93+
onClick: OnClickType
5994
actions?: Array<{
6095
type: ActionType
6196
displayText: {
@@ -119,12 +154,14 @@ export const NotificationsStateConstructor: TypeConstructor<NotificationsState>
119154

120155
export type NotificationType = keyof Omit<NotificationsState, 'dismissed' | 'newlyReceived'>
121156

157+
type AuthType = 'credentials' | 'builderId' | 'identityCenter' | 'unknown'
158+
122159
export interface RuleContext {
123160
readonly ideVersion: typeof vscode.version
124161
readonly extensionVersion: string
125162
readonly os: OperatingSystem
126163
readonly computeEnv: EnvType
127-
readonly authTypes: ('credentials' | 'builderId' | 'identityCenter' | 'unknown')[]
164+
readonly authTypes: AuthType[]
128165
readonly authRegions: string[]
129166
readonly authStates: AuthStatus[]
130167
readonly authScopes: string[]

‎packages/core/src/test/awsService/ec2/model.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ describe('Ec2ConnectClient', function () {
2424
})
2525

2626
describe('getAttachedIamRole', async function () {
27-
it('only returns role if recieves ARN from instance profile', async function () {
27+
it('only returns role if receives ARN from instance profile', async function () {
2828
let role: IAM.Role | undefined
2929
const getInstanceProfileStub = sinon.stub(Ec2Client.prototype, 'getAttachedIamInstanceProfile')
3030

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -557,7 +557,7 @@ function getValidTestNotification(id: string): ToolkitNotification {
557557
description: 'test',
558558
},
559559
},
560-
onRecieve: 'toast',
560+
onReceive: 'toast',
561561
onClick: {
562562
type: 'openUrl',
563563
url: 'https://aws.amazon.com/visualstudiocode/',
@@ -580,7 +580,7 @@ function getInvalidTestNotification(id: string): ToolkitNotification {
580580
description: 'test',
581581
},
582582
},
583-
onRecieve: 'toast',
583+
onReceive: 'toast',
584584
onClick: { type: 'modal' },
585585
},
586586
}

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ describe('Notifications Rendering', function () {
112112
assert.ok(telemetrySpy.calledOnce)
113113
})
114114

115-
it('executes openURL type button', async function () {
115+
it('executes openUrl type button', async function () {
116116
const testWindow = getTestWindow()
117117
testWindow.onDidShowMessage((message) => {
118118
// Simulate user clicking open URL type
@@ -122,7 +122,7 @@ describe('Notifications Rendering', function () {
122122
await verifyOpenExternalUrl(notification)
123123
})
124124

125-
it('executes openTxt type button', async function () {
125+
it('executes openTextDocument type button', async function () {
126126
const testWindow = getTestWindow()
127127
testWindow.onDidShowMessage((message) => {
128128
// Simulate user clicking open txt type
@@ -148,7 +148,7 @@ function getToastURLTestNotification(): ToolkitNotification {
148148
toastPreview: 'test toast preview',
149149
},
150150
},
151-
onRecieve: 'toast',
151+
onReceive: 'toast',
152152
onClick: {
153153
type: 'openUrl',
154154
url: 'https://aws.amazon.com/visualstudiocode/',
@@ -170,7 +170,7 @@ function getTxtNotification(): ToolkitNotification {
170170
description: 'This is a text document notification.',
171171
},
172172
},
173-
onRecieve: 'toast',
173+
onReceive: 'toast',
174174
onClick: {
175175
type: 'openTextDocument',
176176
},
@@ -191,7 +191,7 @@ function getModalNotification(): ToolkitNotification {
191191
description: 'This is a modal notification.',
192192
},
193193
},
194-
onRecieve: 'modal',
194+
onReceive: 'modal',
195195
onClick: {
196196
type: 'modal',
197197
},
@@ -210,7 +210,7 @@ function getModalNotification(): ToolkitNotification {
210210
},
211211
},
212212
{
213-
type: 'openTxt',
213+
type: 'openTextDocument',
214214
displayText: {
215215
'en-US': 'Read More',
216216
},

‎packages/core/src/test/notifications/resources/emergency/1.x.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
"toastPreview": "Signing into Amazon Q is broken, please try this workaround while we work on releasing a fix."
1515
}
1616
},
17-
"onRecieve": "toast",
17+
"onReceive": "toast",
1818
"onClick": {
1919
"type": "openTextDocument"
2020
},
@@ -34,7 +34,7 @@
3434
"toastPreview": "This version of Amazon Q is currently broken, please update to avoid issues."
3535
}
3636
},
37-
"onRecieve": "toast",
37+
"onReceive": "toast",
3838
"onClick": {
3939
"type": "modal"
4040
},

0 commit comments

Comments
 (0)