fix(settings): endpoints is incorrectly trapped by DevSettings (#3305)

## Problem
Calling `DevSettings.instance.get('endpoints', {})` always treats endpoints as being an enabled setting. This would enable 'dev mode' despite no explicit setting being present.

## Solution
Remove the fragile reference comparison for trapping settings. Explicitly check for set values instead.

Other changes:
* Small refactor on the status bar item to make it more robust
* Dev settings that have been unset can now be detected
* Move throttle so it can be re-used; add tests (TODO: remove lodash)
    * Had to bump @sinonjs/faketimers to make one test work w/ fake timers. It'd work without fakes but might be flaky.

Author: JadenSimon

.changes/next-release/Bug Fix-caeb3b17-655d-42e1-b76d-2bb417794228.json

@@ -0,0 +1,4 @@
+{
+	"type": "Bug Fix",
+	"description": "The AWS status bar item is colored despite there not being anything that needs user attention"
+}

package-lock.json

@@ -3441,7 +3441,7 @@
     "devDependencies": {
         "@aws-toolkits/telemetry": "^1.0.120",
         "@cspotcode/source-map-support": "^0.8.1",
-        "@sinonjs/fake-timers": "^8.1.0",
+        "@sinonjs/fake-timers": "^10.0.2",
         "@types/adm-zip": "^0.4.34",
         "@types/async-lock": "^1.4.0",
         "@types/bytes": "^3.1.0",
@@ -3457,7 +3457,7 @@
         "@types/readline-sync": "^1.4.3",
         "@types/semver": "^7.3.6",
         "@types/sinon": "^10.0.5",
-        "@types/sinonjs__fake-timers": "^8.1.1",
+        "@types/sinonjs__fake-timers": "^8.1.2",
         "@types/tcp-port-used": "^1.0.1",
         "@types/uuid": "^8.3.3",
         "@types/vscode": "1.50.0",

package.json

@@ -6,6 +6,7 @@
 import * as vscode from 'vscode'
 import { ResourceTreeDataProvider, TreeNode } from '../shared/treeview/resourceTreeDataProvider'
 import { isCloud9 } from '../shared/extensionUtilities'
+import { throttle } from '../shared/utilities/functionUtils'
 
 export interface RootNode<T = unknown> extends TreeNode<T> {
     /**
@@ -21,26 +22,6 @@ export interface RootNode<T = unknown> extends TreeNode<T> {
     canShow?(): Promise<boolean> | boolean
 }
 
-function throttle<T>(cb: () => T | Promise<T>, delay: number): () => Promise<T> {
-    let timer: NodeJS.Timeout | undefined
-    let promise: Promise<T> | undefined
-
-    return () => {
-        timer?.refresh()
-
-        return (promise ??= new Promise<T>((resolve, reject) => {
-            timer = setTimeout(async () => {
-                timer = promise = undefined
-                try {
-                    resolve(await cb())
-                } catch (err) {
-                    reject(err)
-                }
-            }, delay)
-        }))
-    }
-}
-
 /**
  * The 'local' explorer is represented as 'Developer Tools' in the UI. We use a different name within
  * source code to differentiate between _Toolkit developers_ and _Toolkit users_.

src/awsexplorer/localExplorer.ts

@@ -13,7 +13,7 @@ import { DevSettings } from '../shared/settings'
 import { Auth, login } from './auth'
 import { getAllConnectionsInUse, onDidChangeConnections } from './secondaryAuth'
 import { codicon, getIcon } from '../shared/icons'
-import { shared } from '../shared/utilities/functionUtils'
+import { throttle } from '../shared/utilities/functionUtils'
 
 const statusbarPriority = 1
 
@@ -26,22 +26,19 @@ export async function initializeAwsCredentialsStatusBarItem(
     statusBarItem.command = login.build().asCommand({ title: 'Login' })
     statusBarItem.show()
 
-    const update = shared(async () => {
-        updateItem(statusBarItem)
-        handleDevSettings(devSettings, statusBarItem)
-    })
+    const update = throttle(() => updateItem(statusBarItem, devSettings))
 
     update()
     context.subscriptions.push(
         statusBarItem,
         onDidChangeConnections(() => update()),
         Auth.instance.onDidChangeActiveConnection(() => update()),
         Auth.instance.onDidChangeConnectionState(() => update()),
-        devSettings.onDidChangeActiveSettings(() => handleDevSettings(devSettings, statusBarItem))
+        devSettings.onDidChangeActiveSettings(() => update())
     )
 }
 
-function handleDevSettings(devSettings: DevSettings, statusBarItem: vscode.StatusBarItem) {
+function handleDevSettings(statusBarItem: vscode.StatusBarItem, devSettings: DevSettings) {
     const developerMode = Object.keys(devSettings.activeSettings)
 
     if (developerMode.length > 0) {
@@ -52,7 +49,7 @@ function handleDevSettings(devSettings: DevSettings, statusBarItem: vscode.Statu
     }
 }
 
-function updateItem(statusBarItem: vscode.StatusBarItem): void {
+function updateItem(statusBarItem: vscode.StatusBarItem, devSettings: DevSettings): void {
     const company = getIdeProperties().company
     const connections = getAllConnectionsInUse(Auth.instance)
     const connectedTooltip = localize(
@@ -88,4 +85,7 @@ function updateItem(statusBarItem: vscode.StatusBarItem): void {
         ? new vscode.ThemeColor('statusBarItem.errorBackground')
         : undefined
     ;(statusBarItem as any).backgroundColor = color
+
+    // Do this last to override the normal behavior
+    handleDevSettings(statusBarItem, devSettings)
 }

src/credentials/statusBarItem.ts

@@ -25,7 +25,7 @@ type Workspace = Pick<typeof vscode.workspace, 'getConfiguration' | 'onDidChange
  */
 export class Settings {
     public constructor(
-        private readonly target = vscode.ConfigurationTarget.Global,
+        private readonly updateTarget = vscode.ConfigurationTarget.Global,
         private readonly scope: vscode.ConfigurationScope | undefined = undefined,
         private readonly workspace: Workspace = vscode.workspace
     ) {}
@@ -59,7 +59,7 @@ export class Settings {
      */
     public async update(key: string, value: unknown): Promise<boolean> {
         try {
-            await this.getConfig().update(key, value, this.target)
+            await this.getConfig().update(key, value, this.updateTarget)
 
             return true
         } catch (error) {
@@ -69,6 +69,20 @@ export class Settings {
         }
     }
 
+    /**
+     * Checks if the key has been set in any non-language scope.
+     */
+    public isSet(key: string, section?: string): boolean {
+        const config = this.getConfig(section)
+        const info = config.inspect(key)
+
+        return (
+            info?.globalValue !== undefined ||
+            info?.workspaceValue !== undefined ||
+            info?.workspaceFolderValue !== undefined
+        )
+    }
+
     /**
      * Returns a scoped "slice" (or "view") of the settings configuration.
      *
@@ -84,13 +98,11 @@ export class Settings {
      * ```
      */
     public getSection(section: string): ResetableMemento {
-        const parts = section.split('.')
-        const targetKey = parts.pop() ?? section
-        const parentSection = parts.join('.')
+        const [targetKey, parentSection] = splitKey(section)
 
         return {
             get: (key, defaultValue?) => this.getConfig(section).get(key, defaultValue),
-            reset: async () => this.getConfig().update(section, undefined, this.target),
+            reset: async () => this.getConfig().update(section, undefined, this.updateTarget),
             update: async (key, value) => {
                 // VS Code's settings API can read nested props but not write to them.
                 // We need to write to the parent if we cannot write to the child.
@@ -99,13 +111,13 @@ export class Settings {
                 // handle this asymmetry with try/catch
 
                 try {
-                    return await this.getConfig(section).update(key, value, this.target)
+                    return await this.getConfig(section).update(key, value, this.updateTarget)
                 } catch (error) {
                     const parent = this.getConfig(parentSection)
                     const val = parent.get(targetKey)
 
                     if (typeof val === 'object') {
-                        return parent.update(targetKey, { ...val, [key]: value }, this.target)
+                        return parent.update(targetKey, { ...val, [key]: value }, this.updateTarget)
                     }
 
                     throw error
@@ -161,6 +173,24 @@ export class Settings {
     }
 }
 
+/**
+ * Splits a key into 'leaf' and 'section' components.
+ *
+ * The leaf is assumed to be the last dot-separated component. Example:
+ *
+ * ```ts
+ * const [leaf, section] = this.split('aws.cloudWatchLogs.limit')
+ * console.log(leaf, section) // aws.cloudWatchLogs limit
+ * ```
+ */
+function splitKey(key: string): [leaf: string, section: string] {
+    const parts = key.split('.')
+    const leaf = parts.pop() ?? key
+    const section = parts.join('.')
+
+    return [leaf, section]
+}
+
 // Keeping a separate function without a return type allows us to infer protected methods
 // TODO(sijaden): we can make this better in TS 4.7
 function createSettingsClass<T extends TypeDescriptor>(section: string, descriptor: T) {
@@ -238,6 +268,10 @@ function createSettingsClass<T extends TypeDescriptor>(section: string, descript
             return vscode.Disposable.from(this.getChangedEmitter(), ...this.disposables).dispose()
         }
 
+        protected isSet(key: keyof Inner & string) {
+            return this.settings.isSet(key, section)
+        }
+
         protected getOrThrow<K extends keyof Inner>(key: K & string, defaultValue?: Inner[K]) {
             const value = this.config.get(key, defaultValue)
 
@@ -594,12 +628,15 @@ export class DevSettings extends Settings.define('aws.dev', devSettings) {
     }
 
     public override get<K extends AwsDevSetting>(key: K, defaultValue: ResolvedDevSettings[K]) {
-        const result = super.get(key, defaultValue)
+        if (!this.isSet(key)) {
+            this.unset(key)
 
-        if (result !== defaultValue) {
-            this.trap(key, result)
+            return defaultValue
         }
 
+        const result = super.get(key, defaultValue)
+        this.trap(key, result)
+
         return result
     }
 
@@ -610,6 +647,13 @@ export class DevSettings extends Settings.define('aws.dev', devSettings) {
         }
     }
 
+    private unset(key: AwsDevSetting) {
+        if (key in this.trappedSettings) {
+            delete this.trappedSettings[key]
+            this.onDidChangeActiveSettingsEmitter.fire()
+        }
+    }
+
     static #instance: DevSettings
 
     public static get instance() {

src/shared/settings.ts

@@ -3,6 +3,8 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+import globals from '../extensionGlobals'
+
 /**
  * Creates a function that always returns a 'shared' Promise.
  *
@@ -54,3 +56,32 @@ export function memoize<T, U extends any[]>(fn: (...args: U) => T): (...args: U)
 
     return (...args) => (cache[args.map(String).join(':')] ??= fn(...args))
 }
+
+/**
+ * Prevents a function from executing until {@link delay} milliseconds have passed
+ * since the last invocation. Omitting {@link delay} will throttle the function for
+ * a single event loop.
+ *
+ * Multiple calls made during the throttle window will receive references to the
+ * same Promise similar to {@link shared}. The window will also be 'rolled', delaying
+ * the execution by another {@link delay} milliseconds.
+ */
+export function throttle<T>(cb: () => T | Promise<T>, delay: number = 0): () => Promise<T> {
+    let timer: NodeJS.Timeout | undefined
+    let promise: Promise<T> | undefined
+
+    return () => {
+        timer?.refresh()
+
+        return (promise ??= new Promise<T>((resolve, reject) => {
+            timer = globals.clock.setTimeout(async () => {
+                timer = promise = undefined
+                try {
+                    resolve(await cb())
+                } catch (err) {
+                    reject(err)
+                }
+            }, delay)
+        }))
+    }
+}

src/shared/utilities/functionUtils.ts

@@ -275,7 +275,6 @@ export async function waitTimeout<T, R = void, B extends boolean = true>(
  *
  * Attempts to use the extension-scoped `setTimeout` if it exists, otherwise will fallback to the global scheduler.
  */
-
 export function sleep(duration: number = 0): Promise<void> {
     const schedule = globals?.clock?.setTimeout ?? setTimeout
     return new Promise(r => schedule(r, Math.max(duration, 0)))