Page MenuHomePhabricator

[Cashtab Extension] Close open pop-ups before opening a new one
ClosedPublic

Authored by bytesofman on Oct 26 2023, 22:37.

Details

Summary

Depends on D14692

T3312

Before opening an extension pop-up window, close any existing extension popup windows

This prevents a user issue experienced now that PayButton is live, where a user may wish to complete multiple transactions, and end up with multiple identical small windows open.

Test Plan

npm run extension
load unpacked extension into brave or chrome
visit components.cashtab.com
Click on different Cashtab Badge buttons and observe that you do not have more than one popup open at any given time

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien added inline comments.
cashtab/extension/src/service_worker.js
6 ↗(On Diff #42829)

What is this ? Is it guaranteed to remain the same after a version bump for example ?

164 ↗(On Diff #42829)

This is a bit hacky, can't we have a better option to identify these notification windows ?

bytesofman added inline comments.
cashtab/extension/src/service_worker.js
6 ↗(On Diff #42829)

Yes, these are guaranteed to stay the same. The important one is the PROD_ID which is always the same in the app store.

It's a bit hacky that I am always checking the dev id. Would be better to have some kind of env variable to process these at build time. However, imo the downside is only this extra string check, only when new windows are opened, and I am planning a separate diff to improve extension building using env variables.

imo getting this window fix available and preventing user confusion / unlimited windows is higher priority than maximizing elegance, which extends to some areas outside of this diff's scope.

164 ↗(On Diff #42829)

Including the extension id in the url would be enough --- except for the one case of "the user has the extension open in a tab" -- a common case if the user is running the extension and has clicked this option in the top right corner.

It would be extremely unlikely though for the user to

  1. Open the extension in its own tab
  2. Move this tab to its own window with no other tabs
  3. Resize it to exactly the dimensions of extension-created windows

...even if this does happen, and the extension closes this window...well, this is annoying, but it doesn't destroy any data, just closes a window.

There may be more elegant ways to do this in general -- for example I could keep some kind of registry of all windows the extension has opened and what they were opened for. However imo this is overkill at this stage of development. This type of system would require more thought about window mgmt and also different communication style with windows (instead of triggering features with a query string).

The user problem of "why do I have 30 windows opened after trying this app" should be solved immediately, and this fix, while a bit hacky, does not really introduce technical debt (migrating to some kind of storage api database of windows would be a new "isCashtabWindow" function either way).

With PayButton and more apps coming online, the extension will get more attention from user feedback.

cashtab/extension/src/service_worker.js
164 ↗(On Diff #42829)

I was thinking something much simpler, like adding a property to the pop-up windows on creation like window.killme = true then check for window.killme === true before closing it

bytesofman added inline comments.
cashtab/extension/src/service_worker.js
164 ↗(On Diff #42829)

image.png (578×624 px, 64 KB)

Potentially could be done with the groupId field of the tab -- however, this is just a number, and I'm not really sure what the collision risk is. Also not sure what other impact putting these tabs into a group has, documentation at least for chrome extension dev is not super clear here.

https://developer.chrome.com/docs/extensions/reference/tabs/#method-group

The window create method does not support adding a custom key, and the keys that are supported are all related to browser functionality.

https://developer.chrome.com/docs/extensions/reference/windows/#method-create

Potentially could set custom tabIds, but that is also just a number, and would raise collision possibilities, need to increment for other windows.

I could also inject some sort of code into each newly created window, and then check for that. However, once we are at this stage of complexity, should start considering a more generalized window management system.

For now I think this is the best way to patch the existing issue. Next steps will be

  • Refactor existing code to use promises everywhere (e.g. await extension.windows.getAll...) -- since this is supported in manifest v3 and not v2
  • Better building control of npm run extension using env variables to control what code is in the extension vs what code is in Cashtab web app (stop using some distinct files for the extension)
  • More extension features depending on paybutton / user demand
  • As complexity grows, better window management if and when there is a need
This revision is now accepted and ready to land.Oct 28 2023, 08:13