feat(notification): misc logging, types, fixes #6004
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
We have both ActiveExtensions (checks what extensions are installed and active), and InstalledExtensions (just what extensions are installed). This removes InstalledExtensions because most of the intention behind it is captured by ActiveExtensions. When would we only care about whats installed but not active? Removing a criteria decreases complexity and confusion in both code and for those drafting notifications, so this is an easy removal.
hayemaxi
force-pushed
the
notifications-3
branch
3 times, most recently
from
0735573 to
5661ec0
Compare
Telemetry needs time to be sent, maybe other cleanup routines as well. Also, make sure to flush telemetry before reloading so it isn't lost.
Problem: Making the icon red for emergency notifications requires changing the color property. To do it in 1 line we use a ternary statement and unwrap. This causes runtime error similar to: `cannot access property of undefined: 0`. Solution: Use object.assign instead
- Remove description from the notifications panel - unneeded. It is obvious the panel is for notifications. - Make hover text the full title of the notification in case user side bar width isn't enough to display the full title. - Add a badge to the Q activity bar icon indicating how many notifications you have. Also adds this as (#) to the panel title.
hayemaxi
force-pushed
the
notifications-3
branch
from
5661ec0 to
860b441
Compare
hayemaxi
force-pushed
the
notifications-3
branch
from
a9c743e to
f2e6a0e
Compare
- Fix async tests broken by recent commits. - Add setContext and telemetry verification
hayemaxi
force-pushed
the
notifications-3
branch
from
f2e6a0e to
29dd9c2
Compare
| // Give things time to finish executing. | ||
| globals.clock.setTimeout(() => { | ||
| void this.updateAndReload(notification.displayIf.extensionId) | ||
| }, 1000) |
There was a problem hiding this comment.
is there state that can be checked & retried via waitUntil ?
There was a problem hiding this comment.
Some arbitrary amount of telemetry, e.g. a ui_click and the run block it is currently in. Maybe we could check this telemetry state. A tradeoff between complexity <> and potential (unlikely) race conditions.
No, there is an arbitrary amount of async telemetry. It needs to wait for this block to complete at least. Then there may be other telemetry depending on how this notification was invoked (e.g. ui_click)
| */ | ||
| private async flushRecords(): Promise<void> { | ||
| public async flushRecords(): Promise<void> { |
There was a problem hiding this comment.
we have a didReload concept https://github.com/aws/aws-toolkit-vscode-staging/blob/e02cdb442648177d4e08aab331a9379ad57f9345/packages/core/src/shared/extensionGlobals.ts#L151
which handles window reloads. And our telemetry client has a filesystem cache of telemetry events, which are found on vscode start/reload.
Given the above, is it still necessary to expose flushRecords ?
There was a problem hiding this comment.
didReload applies to SAM only, as it checks for the existence of a key set by it. This also means it only applies to toolkit and not Q.
I see now that telemetry writes to a file on deactivation, previously it looked like it only wrote to an in-memory queue. I will need to verify that deactivation definitely runs on this forced window reload as expected.
There was a problem hiding this comment.
deactivation is not being called for a force reload. Very interesting. So, we still need this call.
Add counterpart to setContext wrapper.
Prevents async functions from running at same time, which may cause notifications to fire twice, etc.
Co-authored-by: Justin M. Keyes <[email protected]>
Don't squash
Commit messages are complete.
Summary:
License: I confirm that my contribution is made under the terms of the Apache 2.0 license.