Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#7784: skip waiting for managed storage after initial wait #7790

Merged
merged 17 commits into from
Mar 3, 2024
1 change: 1 addition & 0 deletions src/__mocks__/browserMock.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

export default {
runtime: {
id: "abcxyz",
getManifest: () => ({
homepage_url: "https://www.pixiebrix.com/",
}),
Expand Down
2 changes: 1 addition & 1 deletion src/auth/useLinkState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {
import useAsyncExternalStore from "@/hooks/useAsyncExternalStore";
import type { AsyncState } from "@/types/sliceTypes";

// NOTE: can't share subscribe methods across generators currently for useAsyncExternalStore, because it maintains
// NOTE: can't share subscribe methods across generators currently for useAsyncExternalStore because it maintains
// a map of subscriptions to state controllers. See https://github.com/pixiebrix/pixiebrix-extension/issues/7789
const subscribe = (callback: () => void) => {
addAuthListener(callback);
Expand Down
2 changes: 1 addition & 1 deletion src/auth/usePartnerAuthData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import useAsyncExternalStore from "@/hooks/useAsyncExternalStore";
import type { AsyncState } from "@/types/sliceTypes";
import type { PartnerAuthData } from "@/auth/authTypes";

// NOTE: can't share subscribe methods across generators currently for useAsyncExternalStore, because it maintains
// NOTE: can't share subscribe methods across generators currently for useAsyncExternalStore because it maintains
// a map of subscriptions to state controllers. See https://github.com/pixiebrix/pixiebrix-extension/issues/7789
const subscribe = (callback: () => void) => {
addAuthListener(callback);
Expand Down
15 changes: 0 additions & 15 deletions src/auth/useRequiredPartnerAuth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -239,21 +239,6 @@ function useRequiredPartnerAuth(): RequiredPartnerState {
authMethodOverride === "partner-oauth2" ||
authMethodOverride === "partner-token";

console.debug("useRequiredPartnerAuth", {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accidentally left this in the other PR

partnerAuthState,
hasPartner,
requiresIntegration,
isMeLoading,
isLinkedLoading,
meError,
partnerKey: partner?.theme ?? managedPartnerId,
hasConfiguredIntegration: {
requiresIntegration,
partnerConfiguration,
isMissingPartnerJwt,
},
});

return {
hasPartner,
partnerKey: partner?.theme ?? managedPartnerId,
Expand Down
10 changes: 0 additions & 10 deletions src/background/background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,23 +47,13 @@ import { initRuntimeLogging } from "@/development/runtimeLogging";
import initWalkthroughModalTrigger from "@/background/walkthroughModalTrigger";
import { initSidePanel } from "./sidePanel";
import initRestrictUnauthenticatedUrlAccess from "@/background/restrictUnauthenticatedUrlAccess";
import {
initManagedStorage,
watchDelayedStorageInitialization,
} from "@/store/enterprise/managedStorage";
import { setPlatform } from "@/platform/platformContext";
import backgroundPlatform from "@/background/backgroundPlatform";

// The background "platform" currently is used to execute API requests from Google Sheets/Automation Anywhere.
// In the future, it might also run other background tasks from mods (e.g., background intervals)
setPlatform(backgroundPlatform);

// Try to initialize managed storage as early as possible because it impacts background behavior
// Call watchDelayedStorageInitialization to handle case where storage is not immediately available within timeout.
// We might consider putting watchStorageInitialization in initManagedStorage, but having a non-terminating
// interval complicates testing.
void initManagedStorage().then(async () => watchDelayedStorageInitialization());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to installer


void initLocator();
void initMessengerLogging();
void initRuntimeLogging();
Expand Down
2 changes: 1 addition & 1 deletion src/background/deploymentUpdater.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ beforeEach(async () => {
organizationId: "00000000-00000000-00000000-00000000",
} as any);

resetManagedStorage();
await resetManagedStorage();
});

describe("updateDeployments", () => {
Expand Down
12 changes: 6 additions & 6 deletions src/background/installer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import {
requirePartnerAuth,
openInstallPage,
handleInstall,
showInstallPage,
} from "@/background/installer";
import * as auth from "@/auth/authStorage";
import { locator } from "@/background/locator";
Expand Down Expand Up @@ -56,9 +56,9 @@ const getUserData = jest.mocked(auth.getUserData);
const locateAllForServiceMock = jest.mocked(locator.locateAllForService);
const browserManagedStorageMock = jest.mocked(browser.storage.managed.get);

afterEach(() => {
afterEach(async () => {
jest.clearAllMocks();
resetManagedStorage();
await resetManagedStorage();
});

describe("openInstallPage", () => {
Expand Down Expand Up @@ -230,7 +230,7 @@ describe("handleInstall", () => {
// App setup tab isn't open
queryTabsMock.mockResolvedValue([]);
isLinkedMock.mockResolvedValue(false);
await handleInstall({
await showInstallPage({
reason: "install",
previousVersion: undefined,
temporary: false,
Expand All @@ -243,7 +243,7 @@ describe("handleInstall", () => {
// App setup tab isn't open
queryTabsMock.mockResolvedValue([]);
isLinkedMock.mockResolvedValue(true);
await handleInstall({
await showInstallPage({
reason: "install",
previousVersion: undefined,
temporary: false,
Expand All @@ -260,7 +260,7 @@ describe("handleInstall", () => {
});
queryTabsMock.mockResolvedValue([]);
isLinkedMock.mockResolvedValue(false);
await handleInstall({
await showInstallPage({
reason: "install",
previousVersion: undefined,
temporary: false,
Expand Down
50 changes: 41 additions & 9 deletions src/background/installer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,17 @@ import { isCommunityControlRoom } from "@/contrib/automationanywhere/aaUtils";
import { isEmpty } from "lodash";
import { expectContext } from "@/utils/expectContext";
import {
readManagedStorage,
initManagedStorage,
isInitialized as isManagedStorageInitialized,
readManagedStorage,
resetInitializationTimestamp as resetManagedStorageInitializationState,
watchForDelayedStorageInitialization,
} from "@/store/enterprise/managedStorage";
import { Events } from "@/telemetry/events";

import { DEFAULT_SERVICE_URL, UNINSTALL_URL } from "@/urlConstants";

import { CONTROL_ROOM_TOKEN_INTEGRATION_ID } from "@/integrations/constants";
import { getExtensionConsoleUrl } from "@/utils/extensionUtils";
import { oncePerSession } from "@/mv3/SessionStorage";

/**
* The latest version of PixieBrix available in the Chrome Web Store, or null if the version hasn't been fetched.
Expand Down Expand Up @@ -213,7 +215,7 @@ export async function requirePartnerAuth(): Promise<void> {
}

// Exported for testing
export async function handleInstall({
export async function showInstallPage({
reason,
previousVersion,
}: Runtime.OnInstalledDetailsType): Promise<void> {
Expand Down Expand Up @@ -291,7 +293,9 @@ export async function handleInstall({
}
}

function onUpdateAvailable({ version }: Runtime.OnUpdateAvailableDetailsType) {
function setAvailableVersion({
version,
}: Runtime.OnUpdateAvailableDetailsType): void {
_availableVersion = version;
}

Expand Down Expand Up @@ -320,10 +324,38 @@ async function setUninstallURL(): Promise<void> {
await browser.runtime.setUninstallURL(url.href);
}

function initInstaller() {
browser.runtime.onUpdateAvailable.addListener(onUpdateAvailable);
browser.runtime.onInstalled.addListener(handleInstall);
browser.runtime.onStartup.addListener(initTelemetry);
// Using our own session value vs. webext-events because onExtensionStart has a 100ms delay
// https://github.com/fregante/webext-events/blob/main/source/on-extension-start.ts#L56
// eslint-disable-next-line local-rules/persistBackgroundData -- using SessionMp via oncePerSession
const initManagedStorageOnce = oncePerSession(
"initManagedStorage",
import.meta.url,
async () => {
await resetManagedStorageInitializationState();
await initManagedStorage();
void watchForDelayedStorageInitialization();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Very readable now

},
);

// eslint-disable-next-line local-rules/persistBackgroundData -- using SessionMp via oncePerSession
const initTelemetryOnce = oncePerSession(
"initTelemetry",
import.meta.url,
async () => {
if (await isLinked()) {
// Init telemetry is a no-op if not linked. So calling without being linked just delays the throttle
await initTelemetry();
}
},
);

function initInstaller(): void {
void initManagedStorageOnce();
void initTelemetryOnce();

browser.runtime.onInstalled.addListener(showInstallPage);
browser.runtime.onUpdateAvailable.addListener(setAvailableVersion);

dntConfig.onChanged(() => {
void setUninstallURL();
});
Expand Down
2 changes: 1 addition & 1 deletion src/background/restrictUnauthenticatedUrlAccess.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ describe("enforceAuthentication", () => {

afterEach(async () => {
// eslint-disable-next-line new-cap -- used for testing
INTERNAL_reset();
await INTERNAL_reset();
await browser.storage.managed.clear();
await browser.storage.local.clear();
axiosMock.reset();
Expand Down
8 changes: 4 additions & 4 deletions src/background/telemetry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

import { type JsonObject } from "type-fest";
import { compact, debounce, throttle, uniq } from "lodash";
import { isLinked } from "@/auth/authStorage";
import { getModComponentState } from "@/store/extensionsStorage";
import {
getLinkedApiClient,
Expand Down Expand Up @@ -305,8 +304,9 @@ async function collectUserSummary(): Promise<UserSummary> {
}

async function init(): Promise<void> {
if ((await isLinked()) && (await allowsTrack())) {
const client = await getLinkedApiClient();
const client = await maybeGetLinkedApiClient();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix race condition


if (client && (await allowsTrack())) {
await client.post("/api/identify/", {
uid: await getUUID(),
data: await collectUserSummary(),
Expand All @@ -321,7 +321,7 @@ export const initTelemetry = throttle(init, 30 * 60 * 1000, {
});

/**
* @deprecated Only allowed in @/background files. Otherwise use: `import reportEvent from "@/telemetry/reportEvent"`
* @deprecated Only allowed in @/background files. Otherwise, use: `import reportEvent from "@/telemetry/reportEvent"`
*/
export async function recordEvent({
event,
Expand Down
2 changes: 0 additions & 2 deletions src/components/logViewer/LogTable.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ import { InputValidationError } from "@/bricks/errors";
import { type Schema } from "@/types/schemaTypes";
import type { LogEntry } from "@/telemetry/logging";

Object.assign(global, { chrome: { runtime: { id: 42 } } });

export default {
title: "Editor/LogTable",
component: LogTable,
Expand Down
2 changes: 1 addition & 1 deletion src/extensionConsole/Navbar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ import useLinkState from "@/auth/useLinkState";
import { DEFAULT_SERVICE_URL } from "@/urlConstants";
import useAsyncExternalStore from "@/hooks/useAsyncExternalStore";

// NOTE: can't share subscribe methods across generators currently for useAsyncExternalStore, because it maintains
// NOTE: can't share subscribe methods across generators currently for useAsyncExternalStore because it maintains
// a map of subscriptions to state controllers. See https://github.com/pixiebrix/pixiebrix-extension/issues/7789
const subscribe = (callback: () => void) => {
addAuthListener(callback);
Expand Down
2 changes: 1 addition & 1 deletion src/extensionConsole/pages/BrowserBanner.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import { INTERNAL_reset } from "@/store/enterprise/managedStorage";

beforeEach(async () => {
// eslint-disable-next-line new-cap -- test helper method
INTERNAL_reset();
await INTERNAL_reset();
await browser.storage.managed.clear();
});

Expand Down
2 changes: 1 addition & 1 deletion src/extensionConsole/pages/onboarding/SetupPage.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ jest.mock("@/data/service/baseService", () => ({

beforeEach(async () => {
jest.clearAllMocks();
resetManagedStorage();
await resetManagedStorage();
await browser.storage.managed.clear();
});

Expand Down
30 changes: 30 additions & 0 deletions src/mv3/SessionStorage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { expectContext } from "@/utils/expectContext";
import { type OmitIndexSignature, type JsonValue } from "type-fest";
import { type ManualStorageKey } from "@/utils/storageUtils";
import { once } from "lodash";
import pMemoize from "p-memoize";

// Just like chrome.storage.session, this must be "global"
// eslint-disable-next-line local-rules/persistBackgroundData -- MV2-only
Expand Down Expand Up @@ -58,6 +59,19 @@ export class SessionMap<Value extends JsonValue> {
return `${this.key}::${this.url}::${secondaryKey}` as ManualStorageKey;
}

async has(secondaryKey: string): Promise<boolean> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

has is required to be used a cache

this.validateContext();
const rawStorageKey = this.getRawStorageKey(secondaryKey);
if (!hasSession) {
return storage.has(rawStorageKey);
}

const result = await browser.storage.session.get(rawStorageKey);
// OK to use `undefined` because undefined is not a valid storage value
// eslint-disable-next-line security/detect-object-injection -- `getRawStorageKey` ensures the format
return result[rawStorageKey] !== undefined;
}

async get(secondaryKey: string): Promise<Value | undefined> {
this.validateContext();
const rawStorageKey = this.getRawStorageKey(secondaryKey);
Expand Down Expand Up @@ -113,3 +127,19 @@ export class SessionValue<Value extends OmitIndexSignature<JsonValue>> {
await this.map.set("#value", value);
}
}

/**
* Helper to run a method at most once per session from the background page.
* @param key the SessionMap key
* @param url the ImportMeta.url of the file
* @param fn the function to run once per session
*/
export const oncePerSession = (
key: string,
url: string,
fn: () => Promise<unknown>,
) =>
pMemoize(fn, {
// `fn` has no arguments, so only one value is stored
cache: new SessionMap(key, url),
});
20 changes: 19 additions & 1 deletion src/store/enterprise/managedStorage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,25 +16,43 @@
*/

import {
initializationTimestamp,
INTERNAL_reset,
readManagedStorage,
readManagedStorageByKey,
} from "@/store/enterprise/managedStorage";
import type { Timestamp } from "@/types/stringTypes";

beforeEach(async () => {
jest.clearAllMocks();
// eslint-disable-next-line new-cap -- test helper method
INTERNAL_reset();
await INTERNAL_reset();
await browser.storage.managed.clear();
});

describe("readManagedStorage", () => {
it("reads immediately if managed storage is already initialized", async () => {
await initializationTimestamp.set(new Date().toISOString() as Timestamp);
await expect(readManagedStorage()).resolves.toStrictEqual({
// `jest-webextension-mock`'s storage is shared across sources, the call ends up with the managed storage
// and the local storage mixed together. See https://github.com/clarkbw/jest-webextension-mock/issues/183
managedStorageInitTimestamp: expect.any(String),
});

// Should only be called once vs. polling
expect(browser.storage.managed.get).toHaveBeenCalledOnce();
});

it("reads uninitialized managed storage", async () => {
await expect(readManagedStorage()).resolves.toStrictEqual({});
});

it("reads managed storage", async () => {
await browser.storage.managed.set({ partnerId: "taco-bell" });
await expect(readManagedStorage()).resolves.toStrictEqual({
// `jest-webextension-mock`'s storage is shared across sources, the call ends up with the managed storage
// and the local storage mixed together. See https://github.com/clarkbw/jest-webextension-mock/issues/183
managedStorageInitTimestamp: expect.any(String),
partnerId: "taco-bell",
});
});
Expand Down