From 6918606677d2040759c62451725e35a50409f1d7 Mon Sep 17 00:00:00 2001 From: Will Martin Date: Mon, 26 Aug 2024 16:09:33 -0400 Subject: [PATCH] [PM-3530] browser extension view cache (#10437) Introduces a way to store temporary component state, for the purposes of persisting views between extension popup open and close. --- .../browser/src/background/main.background.ts | 1 + .../view-cache/popup-view-cache.service.ts | 136 +++++++++++ .../popup/view-cache/popup-view-cache.spec.ts | 224 ++++++++++++++++++ .../popup-view-cache-background.service.ts | 38 ++- apps/browser/src/popup/app.component.ts | 6 +- .../src/popup/services/services.module.ts | 7 + .../abstractions/view-cache.service.ts | 83 +++++++ .../services/noop-view-cache.service.ts | 33 +++ .../src/services/jslib-services.module.ts | 7 + 9 files changed, 532 insertions(+), 3 deletions(-) create mode 100644 apps/browser/src/platform/popup/view-cache/popup-view-cache.service.ts create mode 100644 apps/browser/src/platform/popup/view-cache/popup-view-cache.spec.ts create mode 100644 libs/angular/src/platform/abstractions/view-cache.service.ts create mode 100644 libs/angular/src/platform/services/noop-view-cache.service.ts diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 4081e0e0cd..40e9c8551b 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -577,6 +577,7 @@ export default class MainBackground { ); this.popupViewCacheBackgroundService = new PopupViewCacheBackgroundService( + messageListener, this.globalStateProvider, ); diff --git a/apps/browser/src/platform/popup/view-cache/popup-view-cache.service.ts b/apps/browser/src/platform/popup/view-cache/popup-view-cache.service.ts new file mode 100644 index 0000000000..819a2aa3e4 --- /dev/null +++ b/apps/browser/src/platform/popup/view-cache/popup-view-cache.service.ts @@ -0,0 +1,136 @@ +import { + DestroyRef, + effect, + inject, + Injectable, + Injector, + signal, + WritableSignal, +} from "@angular/core"; +import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; +import { FormGroup } from "@angular/forms"; +import { NavigationEnd, Router } from "@angular/router"; +import { filter, firstValueFrom, skip } from "rxjs"; +import { Jsonify } from "type-fest"; + +import { + FormCacheOptions, + SignalCacheOptions, + ViewCacheService, +} from "@bitwarden/angular/platform/abstractions/view-cache.service"; +import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; +import { MessageSender } from "@bitwarden/common/platform/messaging"; +import { GlobalStateProvider } from "@bitwarden/common/platform/state"; + +import { + ClEAR_VIEW_CACHE_COMMAND, + POPUP_VIEW_CACHE_KEY, + SAVE_VIEW_CACHE_COMMAND, +} from "../../services/popup-view-cache-background.service"; + +/** + * Popup implementation of {@link ViewCacheService}. + * + * Persists user changes between popup open and close + */ +@Injectable({ + providedIn: "root", +}) +export class PopupViewCacheService implements ViewCacheService { + private configService = inject(ConfigService); + private globalStateProvider = inject(GlobalStateProvider); + private messageSender = inject(MessageSender); + private router = inject(Router); + + private featureEnabled: boolean; + + private _cache: Record; + private get cache(): Record { + if (!this._cache) { + throw new Error("Dirty View Cache not initialized"); + } + return this._cache; + } + + /** + * Initialize the service. This should only be called once. + */ + async init() { + this.featureEnabled = await this.configService.getFeatureFlag(FeatureFlag.PersistPopupView); + const initialState = this.featureEnabled + ? await firstValueFrom(this.globalStateProvider.get(POPUP_VIEW_CACHE_KEY).state$) + : {}; + this._cache = Object.freeze(initialState ?? {}); + + this.router.events + .pipe( + filter((e) => e instanceof NavigationEnd), + /** Skip the first navigation triggered by `popupRouterCacheGuard` */ + skip(1), + ) + .subscribe(() => this.clearState()); + } + + /** + * @see {@link ViewCacheService.signal} + */ + signal(options: SignalCacheOptions): WritableSignal { + const { + deserializer = (v: Jsonify): T => v as T, + key, + injector = inject(Injector), + initialValue, + } = options; + const cachedValue = this.cache[key] ? deserializer(JSON.parse(this.cache[key])) : initialValue; + const _signal = signal(cachedValue); + + effect( + () => { + this.updateState(key, JSON.stringify(_signal())); + }, + { injector }, + ); + + return _signal; + } + + /** + * @see {@link ViewCacheService.formGroup} + */ + formGroup(options: FormCacheOptions): TFormGroup { + const { control, injector } = options; + + const _signal = this.signal({ + ...options, + initialValue: control.getRawValue(), + }); + + const value = _signal(); + if (value !== undefined && JSON.stringify(value) !== JSON.stringify(control.getRawValue())) { + control.setValue(value); + control.markAsDirty(); + } + + control.valueChanges.pipe(takeUntilDestroyed(injector?.get(DestroyRef))).subscribe(() => { + _signal.set(control.getRawValue()); + }); + + return control; + } + + private updateState(key: string, value: string) { + if (!this.featureEnabled) { + return; + } + + this.messageSender.send(SAVE_VIEW_CACHE_COMMAND, { + key, + value, + }); + } + + private clearState() { + this.messageSender.send(ClEAR_VIEW_CACHE_COMMAND, {}); + } +} diff --git a/apps/browser/src/platform/popup/view-cache/popup-view-cache.spec.ts b/apps/browser/src/platform/popup/view-cache/popup-view-cache.spec.ts new file mode 100644 index 0000000000..fbe94bece8 --- /dev/null +++ b/apps/browser/src/platform/popup/view-cache/popup-view-cache.spec.ts @@ -0,0 +1,224 @@ +import { Component, inject, Injector } from "@angular/core"; +import { TestBed } from "@angular/core/testing"; +import { FormControl, FormGroup } from "@angular/forms"; +import { Router } from "@angular/router"; +import { RouterTestingModule } from "@angular/router/testing"; +import { MockProxy, mock } from "jest-mock-extended"; + +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; +import { MessageSender } from "@bitwarden/common/platform/messaging"; +import { GlobalStateProvider } from "@bitwarden/common/platform/state"; +import { FakeGlobalState, FakeGlobalStateProvider } from "@bitwarden/common/spec"; + +import { + ClEAR_VIEW_CACHE_COMMAND, + POPUP_VIEW_CACHE_KEY, + SAVE_VIEW_CACHE_COMMAND, +} from "../../services/popup-view-cache-background.service"; + +import { PopupViewCacheService } from "./popup-view-cache.service"; + +@Component({ template: "" }) +export class EmptyComponent {} + +@Component({ template: "" }) +export class TestComponent { + private viewCacheService = inject(PopupViewCacheService); + + formGroup = this.viewCacheService.formGroup({ + key: "test-form-cache", + control: new FormGroup({ + name: new FormControl("initial name"), + }), + }); + + signal = this.viewCacheService.signal({ + key: "test-signal", + initialValue: "initial signal", + }); +} + +describe("popup view cache", () => { + const configServiceMock = mock(); + let testBed: TestBed; + let service: PopupViewCacheService; + let fakeGlobalState: FakeGlobalState>; + let messageSenderMock: MockProxy; + let router: Router; + + const initServiceWithState = async (state: Record) => { + await fakeGlobalState.update(() => state); + await service.init(); + }; + + beforeEach(async () => { + jest.spyOn(configServiceMock, "getFeatureFlag").mockResolvedValue(true); + messageSenderMock = mock(); + + const fakeGlobalStateProvider = new FakeGlobalStateProvider(); + fakeGlobalState = fakeGlobalStateProvider.getFake(POPUP_VIEW_CACHE_KEY); + + testBed = TestBed.configureTestingModule({ + imports: [ + RouterTestingModule.withRoutes([ + { path: "a", component: EmptyComponent }, + { path: "b", component: EmptyComponent }, + ]), + ], + providers: [ + { provide: GlobalStateProvider, useValue: fakeGlobalStateProvider }, + { provide: MessageSender, useValue: messageSenderMock }, + { provide: ConfigService, useValue: configServiceMock }, + ], + }); + + await testBed.compileComponents(); + + router = testBed.inject(Router); + service = testBed.inject(PopupViewCacheService); + }); + + it("should initialize signal when ran within an injection context", async () => { + await initServiceWithState({}); + + const signal = TestBed.runInInjectionContext(() => + service.signal({ + key: "foo-123", + initialValue: "foo", + }), + ); + + expect(signal()).toBe("foo"); + }); + + it("should initialize signal when provided an injector", async () => { + await initServiceWithState({}); + + const injector = TestBed.inject(Injector); + + const signal = service.signal({ + key: "foo-123", + initialValue: "foo", + injector, + }); + + expect(signal()).toBe("foo"); + }); + + it("should initialize signal from state", async () => { + await initServiceWithState({ "foo-123": JSON.stringify("bar") }); + + const injector = TestBed.inject(Injector); + + const signal = service.signal({ + key: "foo-123", + initialValue: "foo", + injector, + }); + + expect(signal()).toBe("bar"); + }); + + it("should initialize form from state", async () => { + await initServiceWithState({ "test-form-cache": JSON.stringify({ name: "baz" }) }); + + const fixture = TestBed.createComponent(TestComponent); + const component = fixture.componentRef.instance; + expect(component.formGroup.value.name).toBe("baz"); + expect(component.formGroup.dirty).toBe(true); + }); + + it("should not modify form when empty", async () => { + await initServiceWithState({}); + + const fixture = TestBed.createComponent(TestComponent); + const component = fixture.componentRef.instance; + expect(component.formGroup.value.name).toBe("initial name"); + expect(component.formGroup.dirty).toBe(false); + }); + + it("should utilize deserializer", async () => { + await initServiceWithState({ "foo-123": JSON.stringify("bar") }); + + const injector = TestBed.inject(Injector); + + const signal = service.signal({ + key: "foo-123", + initialValue: "foo", + injector, + deserializer: (jsonValue) => "test", + }); + + expect(signal()).toBe("test"); + }); + + it("should not utilize deserializer when empty", async () => { + await initServiceWithState({}); + + const injector = TestBed.inject(Injector); + + const signal = service.signal({ + key: "foo-123", + initialValue: "foo", + injector, + deserializer: (jsonValue) => "test", + }); + + expect(signal()).toBe("foo"); + }); + + it("should send signal updates to message sender", async () => { + await initServiceWithState({}); + + const fixture = TestBed.createComponent(TestComponent); + const component = fixture.componentRef.instance; + component.signal.set("Foobar"); + fixture.detectChanges(); + + expect(messageSenderMock.send).toHaveBeenCalledWith(SAVE_VIEW_CACHE_COMMAND, { + key: "test-signal", + value: JSON.stringify("Foobar"), + }); + }); + + it("should send form updates to message sender", async () => { + await initServiceWithState({}); + + const fixture = TestBed.createComponent(TestComponent); + const component = fixture.componentRef.instance; + component.formGroup.controls.name.setValue("Foobar"); + fixture.detectChanges(); + + expect(messageSenderMock.send).toHaveBeenCalledWith(SAVE_VIEW_CACHE_COMMAND, { + key: "test-form-cache", + value: JSON.stringify({ name: "Foobar" }), + }); + }); + + it("should clear on 2nd navigation", async () => { + await initServiceWithState({}); + + await router.navigate(["a"]); + expect(messageSenderMock.send).toHaveBeenCalledTimes(0); + + await router.navigate(["b"]); + expect(messageSenderMock.send).toHaveBeenCalledWith(ClEAR_VIEW_CACHE_COMMAND, {}); + }); + + it("should ignore cached values when feature flag is off", async () => { + jest.spyOn(configServiceMock, "getFeatureFlag").mockResolvedValue(false); + + await initServiceWithState({ "foo-123": JSON.stringify("bar") }); + + const injector = TestBed.inject(Injector); + + const signal = service.signal({ + key: "foo-123", + initialValue: "foo", + injector, + }); + + // The cached state is ignored + expect(signal()).toBe("foo"); + }); +}); diff --git a/apps/browser/src/platform/services/popup-view-cache-background.service.ts b/apps/browser/src/platform/services/popup-view-cache-background.service.ts index 09099b08c8..c2713f70a1 100644 --- a/apps/browser/src/platform/services/popup-view-cache-background.service.ts +++ b/apps/browser/src/platform/services/popup-view-cache-background.service.ts @@ -1,5 +1,6 @@ -import { switchMap, merge, delay, filter, map } from "rxjs"; +import { switchMap, merge, delay, filter, concatMap, map } from "rxjs"; +import { CommandDefinition, MessageListener } from "@bitwarden/common/platform/messaging"; import { POPUP_VIEW_MEMORY, KeyDefinition, @@ -11,6 +12,15 @@ import { fromChromeEvent } from "../browser/from-chrome-event"; const popupClosedPortName = "new_popup"; +/** We cannot use `UserKeyDefinition` because we must be able to store state when there is no active user. */ +export const POPUP_VIEW_CACHE_KEY = KeyDefinition.record( + POPUP_VIEW_MEMORY, + "popup-view-cache", + { + deserializer: (jsonValue) => jsonValue, + }, +); + export const POPUP_ROUTE_HISTORY_KEY = new KeyDefinition( POPUP_VIEW_MEMORY, "popup-route-history", @@ -19,12 +29,35 @@ export const POPUP_ROUTE_HISTORY_KEY = new KeyDefinition( }, ); +export const SAVE_VIEW_CACHE_COMMAND = new CommandDefinition<{ + key: string; + value: string; +}>("save-view-cache"); + +export const ClEAR_VIEW_CACHE_COMMAND = new CommandDefinition("clear-view-cache"); + export class PopupViewCacheBackgroundService { + private popupViewCacheState = this.globalStateProvider.get(POPUP_VIEW_CACHE_KEY); private popupRouteHistoryState = this.globalStateProvider.get(POPUP_ROUTE_HISTORY_KEY); - constructor(private globalStateProvider: GlobalStateProvider) {} + constructor( + private messageListener: MessageListener, + private globalStateProvider: GlobalStateProvider, + ) {} startObservingTabChanges() { + this.messageListener + .messages$(SAVE_VIEW_CACHE_COMMAND) + .pipe( + concatMap(async ({ key, value }) => + this.popupViewCacheState.update((state) => ({ + ...state, + [key]: value, + })), + ), + ) + .subscribe(); + merge( // on tab changed, excluding extension tabs fromChromeEvent(chrome.tabs.onActivated).pipe( @@ -45,6 +78,7 @@ export class PopupViewCacheBackgroundService { async clearState() { return Promise.all([ + this.popupViewCacheState.update(() => ({}), { shouldUpdate: this.objNotEmpty }), this.popupRouteHistoryState.update(() => [], { shouldUpdate: this.objNotEmpty }), ]); } diff --git a/apps/browser/src/popup/app.component.ts b/apps/browser/src/popup/app.component.ts index 27fc868a9b..65fed72138 100644 --- a/apps/browser/src/popup/app.component.ts +++ b/apps/browser/src/popup/app.component.ts @@ -1,4 +1,4 @@ -import { ChangeDetectorRef, Component, NgZone, OnDestroy, OnInit } from "@angular/core"; +import { ChangeDetectorRef, Component, NgZone, OnDestroy, OnInit, inject } from "@angular/core"; import { NavigationEnd, Router, RouterOutlet } from "@angular/router"; import { Subject, takeUntil, firstValueFrom, concatMap, filter, tap } from "rxjs"; @@ -21,6 +21,7 @@ import { } from "@bitwarden/components"; import { BrowserApi } from "../platform/browser/browser-api"; +import { PopupViewCacheService } from "../platform/popup/view-cache/popup-view-cache.service"; import { initPopupClosedListener } from "../platform/services/popup-view-cache-background.service"; import { BrowserSendStateService } from "../tools/popup/services/browser-send-state.service"; import { VaultBrowserStateService } from "../vault/services/vault-browser-state.service"; @@ -37,6 +38,8 @@ import { DesktopSyncVerificationDialogComponent } from "./components/desktop-syn `, }) export class AppComponent implements OnInit, OnDestroy { + private viewCacheService = inject(PopupViewCacheService); + private lastActivity: Date; private activeUserId: UserId; private recordActivitySubject = new Subject(); @@ -64,6 +67,7 @@ export class AppComponent implements OnInit, OnDestroy { async ngOnInit() { initPopupClosedListener(); + await this.viewCacheService.init(); // Component states must not persist between closing and reopening the popup, otherwise they become dead objects // Clear them aggressively to make sure this doesn't occur diff --git a/apps/browser/src/popup/services/services.module.ts b/apps/browser/src/popup/services/services.module.ts index 7c5e49e741..6830809374 100644 --- a/apps/browser/src/popup/services/services.module.ts +++ b/apps/browser/src/popup/services/services.module.ts @@ -1,6 +1,7 @@ import { APP_INITIALIZER, NgModule, NgZone } from "@angular/core"; import { Subject, merge, of } from "rxjs"; +import { ViewCacheService } from "@bitwarden/angular/platform/abstractions/view-cache.service"; import { AngularThemingService } from "@bitwarden/angular/platform/services/theming/angular-theming.service"; import { SafeProvider, safeProvider } from "@bitwarden/angular/platform/utils/safe-provider"; import { @@ -102,6 +103,7 @@ import { OffscreenDocumentService } from "../../platform/offscreen-document/abst import { DefaultOffscreenDocumentService } from "../../platform/offscreen-document/offscreen-document.service"; import BrowserPopupUtils from "../../platform/popup/browser-popup-utils"; import { BrowserFileDownloadService } from "../../platform/popup/services/browser-file-download.service"; +import { PopupViewCacheService } from "../../platform/popup/view-cache/popup-view-cache.service"; import { ScriptInjectorService } from "../../platform/services/abstractions/script-injector.service"; import { BrowserCryptoService } from "../../platform/services/browser-crypto.service"; import { BrowserEnvironmentService } from "../../platform/services/browser-environment.service"; @@ -305,6 +307,11 @@ const safeProviders: SafeProvider[] = [ provide: AutofillServiceAbstraction, useExisting: AutofillService, }), + safeProvider({ + provide: ViewCacheService, + useExisting: PopupViewCacheService, + deps: [], + }), safeProvider({ provide: AutofillService, deps: [ diff --git a/libs/angular/src/platform/abstractions/view-cache.service.ts b/libs/angular/src/platform/abstractions/view-cache.service.ts new file mode 100644 index 0000000000..0ee09afb81 --- /dev/null +++ b/libs/angular/src/platform/abstractions/view-cache.service.ts @@ -0,0 +1,83 @@ +import { Injector, WritableSignal } from "@angular/core"; +import type { FormGroup } from "@angular/forms"; +import type { Jsonify, JsonValue } from "type-fest"; + +type Deserializer = { + /** + * A function to use to safely convert your type from json to your expected type. + * + * @param jsonValue The JSON object representation of your state. + * @returns The fully typed version of your state. + */ + readonly deserializer?: (jsonValue: Jsonify) => T; +}; + +type BaseCacheOptions = { + /** A unique key for saving the cached value to state */ + key: string; + + /** An optional injector. Required if the method is called outside of an injection context. */ + injector?: Injector; +} & (T extends JsonValue ? Deserializer : Required>); + +export type SignalCacheOptions = BaseCacheOptions & { + /** The initial value for the signal. */ + initialValue: T; +}; + +/** Extract the value type from a FormGroup */ +type FormValue = TFormGroup["value"]; + +export type FormCacheOptions = BaseCacheOptions< + FormValue +> & { + control: TFormGroup; +}; + +/** + * Cache for temporary component state + * + * #### Implementations + * - browser extension popup: used to persist UI between popup open and close + * - all other clients: noop + */ +export abstract class ViewCacheService { + /** + * Create a signal from a previously cached value. Whenever the signal is updated, the new value is saved to the cache. + * + * Non browser extension implementations are noop and return a normal signal. + * + * @returns the created signal + * + * @example + * ```ts + * const mySignal = this.viewCacheService.signal({ + * key: "popup-search-text" + * initialValue: "" + * }); + * ``` + */ + abstract signal(options: SignalCacheOptions): WritableSignal; + + /** + * - Initialize a form from a cached value + * - Save form value to cache when it changes + * - The form is marked dirty if the restored value is not `undefined`. + * + * Non browser extension implementations are noop and return the original form group. + * + * @example + * ```ts + * this.loginDetailsForm = this.viewCacheService.formGroup({ + * key: "vault-login-details-form", + * control: this.formBuilder.group({ + * username: [""], + * email: [""], + * }) + * }); + * ``` + **/ + abstract formGroup( + options: FormCacheOptions, + ): TFormGroup; +} diff --git a/libs/angular/src/platform/services/noop-view-cache.service.ts b/libs/angular/src/platform/services/noop-view-cache.service.ts new file mode 100644 index 0000000000..9953e80b3b --- /dev/null +++ b/libs/angular/src/platform/services/noop-view-cache.service.ts @@ -0,0 +1,33 @@ +import { Injectable, signal, WritableSignal } from "@angular/core"; +import type { FormGroup } from "@angular/forms"; + +import { + FormCacheOptions, + SignalCacheOptions, + ViewCacheService, +} from "../abstractions/view-cache.service"; + +/** + * The functionality of the {@link ViewCacheService} is only needed in the browser extension popup, + * yet is provided to all clients to make sharing components easier. + * + * Non-extension clients use this noop implementation. + * */ +@Injectable({ + providedIn: "root", +}) +export class NoopViewCacheService implements ViewCacheService { + /** + * Return a normal signal. + */ + signal(options: SignalCacheOptions): WritableSignal { + return signal(options.initialValue); + } + + /** + * Return the original form group. + **/ + formGroup(options: FormCacheOptions): TFormGroup { + return options.control; + } +} diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 0997fb6863..851e02c8e0 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -268,8 +268,10 @@ import { } from "@bitwarden/vault-export-core"; import { FormValidationErrorsService as FormValidationErrorsServiceAbstraction } from "../platform/abstractions/form-validation-errors.service"; +import { ViewCacheService } from "../platform/abstractions/view-cache.service"; import { FormValidationErrorsService } from "../platform/services/form-validation-errors.service"; import { LoggingErrorHandler } from "../platform/services/logging-error-handler"; +import { NoopViewCacheService } from "../platform/services/noop-view-cache.service"; import { AngularThemingService } from "../platform/services/theming/angular-theming.service"; import { AbstractThemingService } from "../platform/services/theming/theming.service.abstraction"; import { safeProvider, SafeProvider } from "../platform/utils/safe-provider"; @@ -1290,6 +1292,11 @@ const safeProviders: SafeProvider[] = [ useClass: DefaultRegistrationFinishService, deps: [CryptoServiceAbstraction, AccountApiServiceAbstraction], }), + safeProvider({ + provide: ViewCacheService, + useExisting: NoopViewCacheService, + deps: [], + }), ]; @NgModule({