Ensure that we return stable observer references

Signed-off-by: Fredrik Adelöw <freben@gmail.com>

.changeset/sixty-apes-wait.md

@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-shortcuts': patch
+---
+
+Ensure that a stable observable is used in the shortcuts API

packages/core-app-api/api-report.md

@@ -550,6 +550,7 @@ export class UserSettingsStorage implements StorageApi {
     fetchApi: FetchApi;
     discoveryApi: DiscoveryApi;
     errorApi: ErrorApi;
+    identityApi: IdentityApi;
     namespace?: string;
   }): UserSettingsStorage;
   // (undocumented)
@@ -579,7 +580,9 @@ export class WebStorage implements StorageApi {
   // (undocumented)
   get<T>(key: string): T | undefined;
   // (undocumented)
-  observe$<T>(key: string): Observable<StorageValueSnapshot<T>>;
+  observe$<T extends JsonValue>(
+    key: string,
+  ): Observable<StorageValueSnapshot<T>>;
   // (undocumented)
   remove(key: string): Promise<void>;
   // (undocumented)

packages/core-app-api/src/apis/implementations/StorageApi/UserSettingsStorage.test.ts

@@ -18,6 +18,7 @@ import {
   DiscoveryApi,
   ErrorApi,
   FetchApi,
+  IdentityApi,
   StorageApi,
 } from '@backstage/core-plugin-api';
 import { MockFetchApi, setupRequestMockHandlers } from '@backstage/test-utils';
@@ -31,7 +32,12 @@ describe('Persistent Storage API', () => {
 
   const mockBaseUrl = 'http://backstage:9191/api';
   const mockErrorApi = { post: jest.fn(), error$: jest.fn() };
-  const mockDiscoveryApi = { getBaseUrl: async () => mockBaseUrl };
+  const mockDiscoveryApi = {
+    getBaseUrl: async () => mockBaseUrl,
+  };
+  const mockIdentityApi: Partial<IdentityApi> = {
+    getCredentials: async () => ({ token: 'a-token' }),
+  };
 
   const createPersistentStorage = (
     args?: Partial<{
@@ -45,6 +51,7 @@ describe('Persistent Storage API', () => {
       errorApi: mockErrorApi,
       fetchApi: new MockFetchApi(),
       discoveryApi: mockDiscoveryApi,
+      identityApi: mockIdentityApi as IdentityApi,
       ...args,
     });
   };

packages/core-app-api/src/apis/implementations/StorageApi/UserSettingsStorage.ts

@@ -18,12 +18,19 @@ import {
   DiscoveryApi,
   ErrorApi,
   FetchApi,
+  IdentityApi,
   StorageApi,
   StorageValueSnapshot,
 } from '@backstage/core-plugin-api';
 import { ResponseError } from '@backstage/errors';
 import { JsonValue, Observable } from '@backstage/types';
 import ObservableImpl from 'zen-observable';
+import { WebStorage } from './WebStorage';
+
+const JSON_HEADERS = {
+  'Content-Type': 'application/json; charset=utf-8',
+  Accept: 'application/json',
+};
 
 const buckets = new Map<string, UserSettingsStorage>();
 
@@ -38,26 +45,25 @@ export class UserSettingsStorage implements StorageApi {
     ZenObservable.SubscriptionObserver<StorageValueSnapshot<JsonValue>>
   >();
 
-  private readonly observable = new ObservableImpl<
-    StorageValueSnapshot<JsonValue>
-  >(subscriber => {
-    this.subscribers.add(subscriber);
-    return () => {
-      this.subscribers.delete(subscriber);
-    };
-  });
+  private readonly observables = new Map<
+    string,
+    Observable<StorageValueSnapshot<JsonValue>>
+  >();
 
   private constructor(
     private readonly namespace: string,
     private readonly fetchApi: FetchApi,
     private readonly discoveryApi: DiscoveryApi,
     private readonly errorApi: ErrorApi,
+    private readonly identityApi: IdentityApi,
+    private readonly fallback: WebStorage,
   ) {}
 
   static create(options: {
     fetchApi: FetchApi;
     discoveryApi: DiscoveryApi;
     errorApi: ErrorApi;
+    identityApi: IdentityApi;
     namespace?: string;
   }): UserSettingsStorage {
     return new UserSettingsStorage(
@@ -65,6 +71,11 @@ export class UserSettingsStorage implements StorageApi {
       options.fetchApi,
       options.discoveryApi,
       options.errorApi,
+      options.identityApi,
+      WebStorage.create({
+        namespace: options.namespace,
+        errorApi: options.errorApi,
+      }),
     );
   }
 
@@ -80,6 +91,8 @@ export class UserSettingsStorage implements StorageApi {
           this.fetchApi,
           this.discoveryApi,
           this.errorApi,
+          this.identityApi,
+          this.fallback,
         ),
       );
     }
@@ -95,72 +108,81 @@ export class UserSettingsStorage implements StorageApi {
     });
 
     if (!response.ok && response.status !== 404) {
-      throw ResponseError.fromResponse(response);
+      throw await ResponseError.fromResponse(response);
     }
 
-    this.notifyChanges({
-      key,
-      presence: 'absent',
-    });
+    this.notifyChanges({ key, presence: 'absent' });
   }
 
   async set<T extends JsonValue>(key: string, data: T): Promise<void> {
+    if (!(await this.isSignedIn())) {
+      await this.fallback.set(key, data);
+      this.notifyChanges({ key, presence: 'present', value: data });
+    }
+
     const fetchUrl = await this.getFetchUrl(key);
-    const body = JSON.stringify({ value: data });
 
     const response = await this.fetchApi.fetch(fetchUrl, {
       method: 'PUT',
-      body,
+      headers: JSON_HEADERS,
+      body: JSON.stringify({ value: data }),
     });
 
     if (!response.ok) {
-      throw ResponseError.fromResponse(response);
+      throw await ResponseError.fromResponse(response);
     }
 
     const { value } = await response.json();
 
-    this.notifyChanges({
-      key,
-      value,
-      presence: 'present',
-    });
+    this.notifyChanges({ key, value, presence: 'present' });
   }
 
   observe$<T extends JsonValue>(
     key: string,
   ): Observable<StorageValueSnapshot<T>> {
-    // TODO(freben): Introduce server polling or similar, to ensure that different devices update when values change
-    return this.observable.filter(({ key: messageKey }) => messageKey === key);
+    if (!this.observables.has(key)) {
+      this.observables.set(
+        key,
+        new ObservableImpl<StorageValueSnapshot<JsonValue>>(subscriber => {
+          this.subscribers.add(subscriber);
+
+          // TODO(freben): Introduce server polling or similar, to ensure that different devices update when values change
+          Promise.resolve()
+            .then(() => this.get(key))
+            .then(snapshot => subscriber.next(snapshot))
+            .catch(error => this.errorApi.post(error));
+
+          return () => {
+            this.subscribers.delete(subscriber);
+          };
+        }).filter(({ key: messageKey }) => messageKey === key),
+      );
+    }
+
+    return this.observables.get(key) as Observable<StorageValueSnapshot<T>>;
   }
 
   snapshot<T extends JsonValue>(key: string): StorageValueSnapshot<T> {
-    // trigger a reload, ensure it happens on the next tick (after returning)
-    Promise.resolve()
-      .then(() => this.get(key))
-      .then(snapshot => this.notifyChanges(snapshot))
-      .catch(error => this.errorApi.post(error));
-
-    return {
-      key,
-      presence: 'unknown',
-    };
+    return { key, presence: 'unknown' };
   }
 
   private async get<T extends JsonValue>(
     key: string,
   ): Promise<StorageValueSnapshot<T>> {
+    if (!(await this.isSignedIn())) {
+      // This explicitly uses WebStorage, which we know is synchronous and doesn't return presence: unknown
+      return this.fallback.snapshot(key);
+    }
+
     const fetchUrl = await this.getFetchUrl(key);
     const response = await this.fetchApi.fetch(fetchUrl);
 
     if (response.status === 404) {
-      return {
-        key,
-        presence: 'absent',
-      };
+      return { key, presence: 'absent' };
     }
 
     if (!response.ok) {
-      throw ResponseError.fromResponse(response);
+      throw await ResponseError.fromResponse(response);
     }
 
     try {
@@ -172,17 +194,10 @@ export class UserSettingsStorage implements StorageApi {
         return val;
       });
 
-      return {
-        key,
-        presence: 'present',
-        value,
-      };
+      return { key, presence: 'present', value };
     } catch {
       // If the value is not valid JSON, we return an unknown presence. This should never happen
-      return {
-        key,
-        presence: 'absent',
-      };
+      return { key, presence: 'absent' };
     }
   }
 
@@ -204,4 +219,13 @@ export class UserSettingsStorage implements StorageApi {
       }
     }
   }
+
+  private async isSignedIn(): Promise<boolean> {
+    try {
+      const credentials = await this.identityApi.getCredentials();
+      return credentials?.token ? true : false;
+    } catch {
+      return false;
+    }
+  }
 }

packages/core-app-api/src/apis/implementations/StorageApi/WebStorage.ts

@@ -86,7 +86,9 @@ export class WebStorage implements StorageApi {
     this.notifyChanges(key);
   }
 
-  observe$<T>(key: string): Observable<StorageValueSnapshot<T>> {
+  observe$<T extends JsonValue>(
+    key: string,
+  ): Observable<StorageValueSnapshot<T>> {
     return this.observable.filter(({ key: messageKey }) => messageKey === key);
   }
 

plugins/shortcuts/src/api/LocalStoredShortcuts.ts

@@ -27,12 +27,16 @@ import Observable from 'zen-observable';
  * @public
  */
 export class LocalStoredShortcuts implements ShortcutApi {
-  constructor(private readonly storageApi: StorageApi) {}
+  private readonly shortcuts: Observable<Shortcut[]>;
+
+  constructor(private readonly storageApi: StorageApi) {
+    this.shortcuts = Observable.from(
+      this.storageApi.observe$<Shortcut[]>('items'),
+    ).map(snapshot => snapshot.value ?? []);
+  }
 
   shortcut$() {
-    return Observable.from(this.storageApi.observe$<Shortcut[]>('items')).map(
-      snapshot => snapshot.value ?? [],
-    );
+    return this.shortcuts;
   }
 
   get() {

plugins/user-settings-backend/README.md

@@ -35,7 +35,7 @@ export default async function createPlugin(env: PluginEnvironment) {
  // packages/backend/src/index.ts
 +import userSettings from './plugins/userSettings';
  async function main() {
-+  const userSettingsEnv = useHotMemoize(module, () => createEnv('userSettings'));
++  const userSettingsEnv = useHotMemoize(module, () => createEnv('user-settings'));
    const apiRouter = Router();
 +  apiRouter.use('/user-settings', await userSettings(userSettingsEnv));
 }
@@ -52,8 +52,9 @@ To make use of the user settings backend, replace the `WebStorage` with the
    AnyApiFactory,
    createApiFactory,
 +  discoveryApiRef,
-+  fetchApiRef
++  fetchApiRef,
    errorApiRef,
++  identityApi,
 +  storageApiRef,
  } from '@backstage/core-plugin-api';
 +import { UserSettingsStorage } from '@backstage/core-app-api';
@@ -65,9 +66,9 @@ To make use of the user settings backend, replace the `WebStorage` with the
 +      discoveryApi: discoveryApiRef,
 +      errorApi: errorApiRef,
 +      fetchApi: fetchApiRef,
++      identityApi: identityApiRef
 +    },
-+    factory: ({ discoveryApi, errorApi, fetchApi }) =>
-+      UserSettingsStorage.create({ discoveryApi, errorApi, fetchApi }),
++    factory: deps => UserSettingsStorage.create(deps),
 +  }),
  ];
 ```