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

twschiller · 2024-03-02T16:27:52Z

What does this PR do?

Closes Only delay/wait for managed storage once on browser extension startup #7784
Adds a timestamp to local storage to enable contexts to skip waiting for managed storage after the initial delay
Fixes use of onStartup handler in favor of a method that runs once per browser session

Remaining Work

Handle onChanged test failures?:

/Users/tschiller/projects/pixiebrix-extension/node_modules/webext-storage/distribution/storage-item.js:39
        chrome.storage.onChanged.addListener(changeHandler);
                       ^

TypeError: Cannot read properties of undefined (reading 'onChanged')

Fix test failures due to cleanup abort controller cleanup

Reviewer Tips

Review managedStorage.ts
Review installer.ts

Future Work

Figure out how to use jest fake timers during tests to avoid incurring the delay
Submit PR to split storages in the jest-webextension-mock project: Separate storage values by type clarkbw/jest-webextension-mock#183
After switch to MV3, can just store the flag/timestamp in session storage (but the current local storage approach will still work)

Checklist

Add tests
New files added to src/tsconfig.strictNullChecks.json (if possible): N/A
Designate a primary reviewer: @fregante

twschiller · 2024-03-02T16:48:07Z

src/auth/useRequiredPartnerAuth.ts

@@ -239,21 +239,6 @@ function useRequiredPartnerAuth(): RequiredPartnerState {
    authMethodOverride === "partner-oauth2" ||
    authMethodOverride === "partner-token";

-  console.debug("useRequiredPartnerAuth", {


Accidentally left this in the other PR

twschiller · 2024-03-02T17:08:51Z

@fregante thoughts on how to fix onChanged errors in tests?: https://github.com/pixiebrix/pixiebrix-extension/pull/7790/files#diff-da2fdc70c5e193c8b4888da54a7cb919a8bd98a8196358ac0d300b60b18922bbR52

fregante · 2024-03-02T17:47:14Z

Oh it seems that the whole chrome object is being overridden to just { runtime: { id: 42 } }

Dropping this line fixes the issue, but it breaks the LogTable story

pixiebrix-extension/src/components/logViewer/LogTable.stories.tsx

Line 29 in d23a4a0

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

fregante · 2024-03-02T17:54:11Z

src/background/installer.ts

@@ -321,9 +323,14 @@ async function setUninstallURL(): Promise<void> {
 }

 function initInstaller() {
+  browser.runtime.onStartup.addListener(async () => {


Note that onStartup does not mean "on extension start", but "on browser start"

This means that this won't run when the user first installs the extension or when they re-enable it.

In MV2, all code is run at the start.

in MV3, you might want to try https://github.com/fregante/webext-events/blob/main/source/on-extension-start.md

src/testUtils/testAfterEnv.ts

twschiller · 2024-03-02T20:51:42Z

@fregante thanks for the help tracking down the onChanged issue. This should be ready for another review. I ended up using SessionValue instead of onSessionStart to avoid the setInterval

codecov · 2024-03-02T20:55:48Z

Codecov Report

Attention: Patch coverage is 59.59596% with 40 lines in your changes are missing coverage. Please review.

Project coverage is 72.31%. Comparing base (4cd4511) to head (9787f65).
Report is 7 commits behind head on main.

Files	Patch %	Lines
src/store/enterprise/managedStorage.ts	60.78%	20 Missing ⚠️
src/background/installer.ts	23.52%	13 Missing ⚠️
src/mv3/SessionStorage.ts	72.22%	5 Missing ⚠️
src/background/telemetry.ts	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7790      +/-   ##
==========================================
+ Coverage   72.28%   72.31%   +0.03%     
==========================================
  Files        1271     1271              
  Lines       39697    39748      +51     
  Branches     7372     7380       +8     
==========================================
+ Hits        28693    28743      +50     
- Misses      11004    11005       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

twschiller · 2024-03-02T20:57:09Z

src/store/enterprise/useManagedStorageState.ts


 type HookState = {
  data: Nullishable<ManagedStorageState>;
  isLoading: boolean;
 };

+// 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
+function subscribe(callback: () => void): () => void {


Moved here from managedStorage to prevent accidentally using across multiple hooks given the useAsyncExternalStore footgun

src/store/enterprise/managedStorage.ts

fregante · 2024-03-03T03:41:01Z

src/background/installer.ts

+  // 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
+
+  // Can't use onStartup because it doesn't mean "on extension start", but "on browser profile start".
+  // Events registered onStartup won't run when the user first installs or when they re-enable the extension.
+  // See: https://developer.chrome.com/docs/extensions/reference/api/runtime#event-onStartup
+
+  if (await initialized.get()) {
+    // Session already initialized
+    return;
+  }


I'm generally against inlining logic that is reused, e.g. logic like once, memoize, throttle and the whole managed storage initialization logic. Doing otherwise leads to mixing business logic where it's not expected.

I think this call could be:

const initSessionOnce = pMemoize(async () => { // }, { // initSessionOnce has no arguments, so only one value is stored cache: new SessionMap('managedstorageinit', import.meta.url) })

or

const oncePerSession = (key, url, fn) => pMemoize(fn, { cache: new SessionMap(key, url) }) const initSessionOnce = oncePerSession('managedstorageinit', import.meta.url, async () => { // });

src/store/enterprise/managedStorage.ts

fregante · 2024-03-03T03:46:39Z

src/store/enterprise/managedStorage.ts

+    const waitController = new AbortController();
+    // Skip polling if it becomes initialized while waiting
+    initializationTimestamp.onChanged(
+      waitController.abort,


Suggested change

waitController.abort,

waitController.abort.bind(waitController),

But in reality this cannot happen. Line 185 (initializationTimestamp.set) cannot be reached by another process while this one is running because waitForInitialManagedStorage is only run once.

I think that's related to the misunderstanding here? -- that the timestamp is using local storage to be available across contexts: #7790 (comment)

The abort logic in the waitForInitialManagedStorage handles the case where:

Background page starts up and is starts polling for managed storage

Extension Console Page Starts up and starts polling

Background page page finished polling

The new abort controller allows the Extension Console page quit polling because it knows the Background Page already polled the 4.5 seconds

src/utils/promiseUtils.ts

twschiller · 2024-03-03T13:37:53Z

src/background/background.ts

-// 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());


Moved to installer

twschiller · 2024-03-03T13:39:05Z

src/background/telemetry.ts

@@ -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();


Fix race condition

twschiller · 2024-03-03T13:39:26Z

src/mv3/SessionStorage.ts

@@ -58,6 +59,19 @@ export class SessionMap<Value extends JsonValue> {
    return `${this.key}::${this.url}::${secondaryKey}` as ManualStorageKey;
  }

+  async has(secondaryKey: string): Promise<boolean> {


has is required to be used a cache

fregante

🙌

fregante · 2024-03-03T15:16:29Z

src/background/installer.ts

+  async () => {
+    await resetManagedStorageInitializationState();
+    await initManagedStorage();
+    void watchForDelayedStorageInitialization();


Nice. Very readable now

fregante · 2024-03-03T15:24:59Z

src/testUtils/testAfterEnv.ts

 jest.setMock("webext-detect-page", detectPageMock);
+
+// For some reason, throwIfAborted is not available in Jest environment even though it appears to be in JSDOM
+// https://github.com/jsdom/jsdom/blob/2f8a7302a43fff92f244d5f3426367a8eb2b8896/lib/jsdom/living/aborting/AbortSignal-impl.js#L24


Yeah it's unfortunate. More info in FixJsdomEnvironment.js and testEnv.js

https://github.com/pixiebrix/pixiebrix-extension/blob/6c95cf428c1508a16413a9e59549a2c636ff56cc/src/testUtils/testEnv.js#L44-L55

github-actions · 2024-03-03T15:26:48Z

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

#7784: skip waiting for managed storage after initial wait

c48d40b

twschiller requested review from fregante and fungairino March 2, 2024 16:27

twschiller assigned twschiller and fregante Mar 2, 2024

twschiller added 2 commits March 2, 2024 11:35

Add tests for pollForTruthy

8dcf1dc

Fix lint

f2d1f7c

vercel bot deployed to Preview March 2, 2024 16:39 View deployment

Fix snapshot failure

ea5e847

vercel bot deployed to Preview March 2, 2024 16:44 View deployment

Add test

7196816

twschiller added this to the 1.8.10 milestone Mar 2, 2024

twschiller added the performance label Mar 2, 2024

twschiller commented Mar 2, 2024

View reviewed changes

vercel bot deployed to Preview March 2, 2024 16:49 View deployment

Try to fix test errors

d23a4a0

vercel bot deployed to Preview March 2, 2024 17:09 View deployment

This comment was marked as outdated.

Sign in to view

fregante reviewed Mar 2, 2024

View reviewed changes

Fix session startup code

dc1a0e0

vercel bot deployed to Preview March 2, 2024 20:49 View deployment

twschiller requested a review from grahamlangford March 2, 2024 20:54

twschiller commented Mar 2, 2024

View reviewed changes

fregante reviewed Mar 3, 2024

View reviewed changes

src/store/enterprise/managedStorage.ts Show resolved Hide resolved

fregante reviewed Mar 3, 2024

View reviewed changes

Address PR comments

f9dec79

vercel bot deployed to Preview March 3, 2024 13:28 View deployment

Include test signal in aborting poll for truthy

185ca73

vercel bot deployed to Preview March 3, 2024 13:33 View deployment

Add method comment

067213f

vercel bot deployed to Preview March 3, 2024 13:36 View deployment

Fix has check

1884d36

twschiller commented Mar 3, 2024

View reviewed changes

vercel bot deployed to Preview March 3, 2024 13:38 View deployment

twschiller commented Mar 3, 2024

View reviewed changes

Fix session map behavior

671709f

vercel bot deployed to Preview March 3, 2024 13:58 View deployment

Fix signal bug

009cdbf

vercel bot deployed to Preview March 3, 2024 14:12 View deployment

Fix comment

5453e13

vercel bot deployed to Preview March 3, 2024 14:14 View deployment

Fix broken test

700b1fc

vercel bot deployed to Preview March 3, 2024 14:49 View deployment

Fix test reset

517a06e

vercel bot deployed to Preview March 3, 2024 15:15 View deployment

fregante approved these changes Mar 3, 2024

View reviewed changes

Co-locate AbortSignal polyfills

9787f65

vercel bot deployed to Preview March 3, 2024 20:58 View deployment

twschiller merged commit 4878e51 into main Mar 3, 2024
16 checks passed

twschiller deleted the feature/7784-managed-storage-delay branch March 3, 2024 21:05

twschiller mentioned this pull request Mar 5, 2024

"Registry Id not Found" race condition on mod activation on first run #7822

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

twschiller commented Mar 2, 2024 •

edited

twschiller Mar 2, 2024

twschiller commented Mar 2, 2024 •

edited

This comment was marked as outdated.

fregante commented Mar 2, 2024 •

edited

fregante Mar 2, 2024

twschiller commented Mar 2, 2024

codecov bot commented Mar 2, 2024 •

edited

twschiller Mar 2, 2024 •

edited

fregante Mar 3, 2024

fregante Mar 3, 2024

twschiller Mar 3, 2024

twschiller Mar 3, 2024

twschiller Mar 3, 2024

twschiller Mar 3, 2024

fregante left a comment

fregante Mar 3, 2024

fregante Mar 3, 2024 •

edited

github-actions bot commented Mar 3, 2024

	waitController.abort,
	waitController.abort.bind(waitController),

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

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

Conversation

twschiller commented Mar 2, 2024 • edited

What does this PR do?

Remaining Work

Reviewer Tips

Future Work

Checklist

Choose a reason for hiding this comment

twschiller commented Mar 2, 2024 • edited

This comment was marked as outdated.

fregante commented Mar 2, 2024 • edited

Choose a reason for hiding this comment

twschiller commented Mar 2, 2024

codecov bot commented Mar 2, 2024 • edited

Codecov Report

twschiller Mar 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante Mar 3, 2024 • edited

Choose a reason for hiding this comment

github-actions bot commented Mar 3, 2024

twschiller commented Mar 2, 2024 •

edited

twschiller commented Mar 2, 2024 •

edited

fregante commented Mar 2, 2024 •

edited

codecov bot commented Mar 2, 2024 •

edited

twschiller Mar 2, 2024 •

edited

fregante Mar 3, 2024 •

edited