Add onUpdate event listener for installing starter blueprints on Playground urls #4249

mnholtz · 2022-09-07T17:35:00Z

What does this PR do?

Closes Create starter blueprints install link #4245
Depends on https://github.com/pixiebrix/pixiebrix-app/pull/2250
Instead of implementing an installation link for Playground blueprints (as described in the issue), this adds an onUpdate event listener on browser tabs that will install starter blueprints if the user visits/refreshes a Playground page.
We are still "preinstalling" Starter Blueprints & opening the Playground on first-time installs if successful just in case this process fails

Reviewer Tips

Take a look mainly at changes to starterBlueprints.ts. Lots of additional refactoring was done to support this feature. The tests in starterBlueprint.test.ts describe expected behavior for first-time installs.

Discussion

I've added an installation lock and a debounce to the onUpdate event listener for Playground installation, in order to prevent issues with the onUpdate event being fired rapidly (as it often does).

Demo

Screen.Recording.2022-09-07.at.9.26.27.AM.mov

Future Work

We decided that we'd like to modify the Webflow Playground pages to include a more helpful message if the Playground blueprint installation still fails.

Checklist

Add tests
Designate a primary reviewer @fregante

…iebrix-extension into feature/4245_playground_recovery a commit message to explain why this merge is necessary, f it merges an updated upstream into a topic branch. ng with '#' will be ignored, and an empty message aborts fix test typo

BLoe

The naming here is slightly hard to follow, but it's not too bad and it's all contained in this file, nothing is exported. You could possibly add some jsdoc comments on the functions to help explain the flow of things, and also to clarify what the boolean return values mean.

BLoe · 2022-09-07T18:26:03Z

src/background/starterBlueprints.ts

-    let extensionsState = await loadOptions();
+    return shouldInstall;
+  } catch (error) {
+    reportError(error);


What is going to be returned from this function when it catches an error? I think void will be coerced to false, but that's not really clear from looking at the code. Can we please return false here explicitly to make it more clear?

👍 TypeScript in loose mode misses a lot of such warnings. It should have complained that the signature is boolean but there's no return here.

Thanks for catching this Ben! This was indeed a mistake.

fregante · 2022-09-07T19:00:31Z

src/background/starterBlueprints.ts

+    console.debug(
+      "Skipping starter blueprint installation because the extension is not linked to the PixieBrix service"
+    );
+    return [];


fregante · 2022-09-08T06:41:01Z

src/background/starterBlueprints.ts

-  void installStarterBlueprints();
+  browser.tabs.onUpdated.addListener((tabId, changeInfo, tab) => {
+    if (tab?.url?.startsWith(PLAYGROUND_URL)) {
+      void installStarterBlueprints();


I think my comment got lost:

This listener will run installStarterBlueprints every time the playground is opened, for any number of open tabs, and every time a new page is visited within the playground. Given it contains some async getter/setters, this will certainly cause trouble (as well as unnecessary computation)

How about this pattern:

async function initStarterBlueprints() { if (await isInstalled()) { return } if (await shouldInstall() || await onPlayGroundOpen()) { install() } } async function onPlayGroundOpen() { return new Promise(resolve => { const listener = (id, info, tab) => { if (tab?.url?.startsWith(PLAYGROUND_URL)) { browser.tabs.onUpdated.removeListener(listener) resolve(true) } } browser.tabs.onUpdated.addListener(listener) }) }

To note:

initStarterBlueprints is only run once and may be "pending" forever, or until the user opens the playground

The function will run, but I think the check on line 54 prevents any blueprint from being installed twice, right? What kind of trouble are you imagining this will cause? Can you explain how your code example here avoids that trouble?

browser.tabs.onUpdated fires a lot of times with all kinds of updates, even favicon updates, status updates, url updates.

installStarterBlueprints is called the first time

It starts an API call via getStarterBlueprints

installStarterBlueprints is called a second time while the API call is still in progress

It starts another API call

At this point line 54 hasn't been reached yet, so any logic there is futile.

My example avoids the trouble because it does call installStarterBlueprints every time the event happens; the listener is only used once to resolve a promise, and then the listener is removed.

What kind of trouble are you imagining this will cause?

Some possibilities:

each client reporting the same error multiple times on line 112

each client making multiple unnecessary calls to the server, further slowing the server down

loadOptions being called twice simultaneously, and then saveOptions overriding each other (race condition, almost exactly what was described in Verify that recordEvent correctly handles concurrent events #4073)

@fregante I'm not sure the pattern you suggested fits the needs of the feature, unless I'm misunderstanding your pseudocode. initStarterBlueprints will run only once per extension launch. Therefore, the conditional if (await shouldInstall() || await onPlayGroundOpen()) { will also be run only once. Subsequent visits to the Playground page after uninstalling Playground blueprints/or a failed installation will not work. I'm actually not sure why the event listener is being re-added in the pseudocode if the conditional is run only once anyways.

We need users to always have Starter Blueprints installed for them when visiting the Playground page, to catch the cases if they were to uninstall the Blueprints and revisit the Playground, or if an error like a network request failed to install them the first time. In this case, I think I might go with a locking (and/or maybe debounce?) pattern in order to solve the concerns you outlined above.

We need users to always have Starter Blueprints installed for them when visiting the Playground page

I didn't think about that, although it would only be hit if they visit the playground, delete the blueprint, and then visit the playground again — all while never quitting the browser.

I guess it's fine to debounce it to once per minute, that would work even better.

Thanks for the discussion Federico! Yeah, debounce sounds good to me also.

Yeah thanks federico! Great considerations here

codecov-commenter · 2022-09-08T18:34:07Z

Codecov Report

Merging #4249 (271a258) into main (63f3851) will increase coverage by 0.05%.
The diff coverage is 85.45%.

❗ Current head 271a258 differs from pull request most recent head f35c539. Consider uploading reports for the commit f35c539 to get more accurate results

@@            Coverage Diff             @@
##             main    #4249      +/-   ##
==========================================
+ Coverage   48.45%   48.51%   +0.05%     
==========================================
  Files         887      887              
  Lines       26105    26143      +38     
  Branches     5391     5397       +6     
==========================================
+ Hits        12650    12684      +34     
- Misses      12537    12540       +3     
- Partials      918      919       +1

Impacted Files	Coverage Δ
src/background/starterBlueprints.ts	`86.11% <85.45%> (-1.39%)`	⬇️
src/options/pages/blueprints/BlueprintsPage.tsx	`0.00% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

fregante

fregante · 2022-09-08T18:56:37Z

src/background/starterBlueprints.ts

    })
  );
 }

-export async function installStarterBlueprints(): Promise<void> {
+async function installBlueprints(


To ensure that it's not called undebounced (as it currently is on line 150), we can prepend _ to the name.

I generally inline the function in the debounce definition (debounce(() => {}, opts)) but for long functions it's less readable.

Suggested change

async function installBlueprints(

async function _installBlueprints(

fregante · 2022-09-08T18:57:56Z

src/background/starterBlueprints.ts


-    await saveOptions(extensionsState);
+  const installed = await installStarterBlueprints();


While unlikely, it could be called concurrently with browser.tabs.onUpdated. It doesn't hurt to use the debounced version everywhere to avoid these leaks

Misha Holtz added 7 commits September 6, 2022 12:56

add tabs onUpdated event listener for starter blueprints

61d9d76

install stater blueprints on playground urls

841cfa8

fix starter blueprint tests

814e831

refactor extract install starter blueprints loop

121ce90

extract more installation methods

44847c0

refactor installStarterBlueprints

e255c71

update starter blueprint onUpdate url

00af9ef

mnholtz requested review from BLoe, fregante and jcforest September 7, 2022 17:35

Misha Holtz and others added 2 commits September 7, 2022 10:48

fix starterBlueprint test

cd48429

Merge branch 'main' into feature/4245_playground_recovery

9da961c

vercel bot deployed to Preview September 7, 2022 17:54 View deployment

mnholtz and others added 3 commits September 7, 2022 11:09

Merge branch 'main' into feature/4245_playground_recovery

381c180

fix test typo

df0222f

vercel bot deployed to Preview September 7, 2022 18:11 View deployment

vercel bot deployed to Preview September 7, 2022 18:14 View deployment

BLoe approved these changes Sep 7, 2022

View reviewed changes

mnholtz changed the title ~~Feature/4245 playground recovery~~ Add onUpdate event listener for installing starter blueprints on Playground urls Sep 7, 2022

fregante suggested changes Sep 8, 2022

View reviewed changes

Misha Holtz added 2 commits September 8, 2022 10:46

add installation lock

0d81973

add debounce to starter blueprint installation for onUpdate events

dc0c521

vercel bot deployed to Preview September 8, 2022 18:26 View deployment

remove trailing param

efcf5df

vercel bot deployed to Preview September 8, 2022 18:32 View deployment

readd trailing

5ba96b4

add boolean return on error

271a258

vercel bot deployed to Preview September 8, 2022 18:39 View deployment

Merge branch 'main' into feature/4245_playground_recovery

f35c539

vercel bot deployed to Preview September 8, 2022 18:46 View deployment

mnholtz requested a review from fregante September 8, 2022 18:53

fregante approved these changes Sep 8, 2022

View reviewed changes

call debouncedInstallStarterBlueprints for first time installs also

c50cdf1

vercel bot deployed to Preview September 8, 2022 22:47 View deployment

Misha Holtz and others added 2 commits September 8, 2022 16:17

fix jest tests for new debounce refactor with fake timers'

0ef4033

Merge branch 'main' into feature/4245_playground_recovery

e72687f

vercel bot deployed to Preview September 8, 2022 23:22 View deployment

mnholtz merged commit 238f5eb into main Sep 8, 2022

mnholtz deleted the feature/4245_playground_recovery branch September 8, 2022 23:36

twschiller added this to the 1.7.7 milestone Sep 14, 2022

mnholtz mentioned this pull request Aug 20, 2024

Race condition waiting for welcome mods to be activated on first run #9014

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add onUpdate event listener for installing starter blueprints on Playground urls #4249

Add onUpdate event listener for installing starter blueprints on Playground urls #4249

mnholtz commented Sep 7, 2022 •

edited

Loading

BLoe left a comment

BLoe Sep 7, 2022

fregante Sep 7, 2022 •

edited

Loading

mnholtz Sep 8, 2022

fregante Sep 7, 2022

fregante Sep 8, 2022

BLoe Sep 8, 2022

fregante Sep 8, 2022

fregante Sep 8, 2022

mnholtz Sep 8, 2022 •

edited

Loading

fregante Sep 8, 2022 •

edited

Loading

BLoe Sep 8, 2022

mnholtz Sep 8, 2022

codecov-commenter commented Sep 8, 2022 •

edited

Loading

fregante left a comment

fregante Sep 8, 2022 •

edited

Loading

fregante Sep 8, 2022 •

edited

Loading

	async function installBlueprints(
	async function _installBlueprints(


		await saveOptions(extensionsState);
		const installed = await installStarterBlueprints();

Add onUpdate event listener for installing starter blueprints on Playground urls #4249

Add onUpdate event listener for installing starter blueprints on Playground urls #4249

Conversation

mnholtz commented Sep 7, 2022 • edited Loading

What does this PR do?

Reviewer Tips

Discussion

Demo

Future Work

Checklist

BLoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante Sep 7, 2022 • edited Loading

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

mnholtz Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

fregante Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 8, 2022 • edited Loading

Codecov Report

fregante left a comment

Choose a reason for hiding this comment

fregante Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

fregante Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

mnholtz commented Sep 7, 2022 •

edited

Loading

fregante Sep 7, 2022 •

edited

Loading

mnholtz Sep 8, 2022 •

edited

Loading

fregante Sep 8, 2022 •

edited

Loading

codecov-commenter commented Sep 8, 2022 •

edited

Loading

fregante Sep 8, 2022 •

edited

Loading

fregante Sep 8, 2022 •

edited

Loading