zeeguu / web

Frontend for the zeeguu web application.
https://www.zeeguu.org
5 stars 8 forks source link

should we improve the definition of `runningInChromeDesktop`? #333

Closed mircealungu closed 5 months ago

mircealungu commented 6 months ago

cf. sentry issue we get exceptions of the following kind:

image

this means, that our runningInChromeDesktop returns true, but we don't have chrome defined. I guess that function is not working as well as it should.

Alternatives to consider:

tfnribeiro commented 6 months ago

Quick question: Isn't this part of the browser extension codebase?

Having a quick look at Sentry, I saw that turns out that the browser originating this is Chrome, so the flag is correct.

The issue seems to be that now, we use the Browser API rather than importing the global chromium.

I have confirmed that the browser API supports the send message: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/runtime/sendMessage

So essentially, we shouldn't even check for the browser, just make a call: BROWSER_API.runtime.sendMessage

I will try to check other places where we have chrome.runtime, as I would guess most should be eliminated and moved to the BROWSER_API handler.

tfnribeiro commented 6 months ago

Looking into this, I saw that currently the BROWSER_API is under the browser_extension, but this method is within the web.

The way I am approaching this, is by creating an utils/extension folder, where I moved the browserApi.js file and now we can import the BROWSER_API and send the message through that. This means we would be able to call it from anywhere in the codebase, either web or the extension.

I also realized that currently we are checking runningInChromeDesktop rather than just chrome, which we might want to update to runningInChrome

export const BROWSER_API = runningInChromeDesktop() ? chrome : browser;

Let me know what you think!

tfnribeiro commented 6 months ago

I have been looking a little more into this, and I get the feeling that these APIs only work with the extensions? I feel like everytime I open documentation it usually comes associated with extension code: https://developer.chrome.com/docs/extensions/reference/api/runtime#method-sendMessage

So I have tried to just set the code that was under the Firefox and that seems to work for chrome as well, and then we would only have one case to handle. Wouldn't this be a better solution?

tfnribeiro commented 6 months ago

So I have looked at this, and my proposed solution is the following:

The extension injects a code on the Zeeguu URLs that essentially sends a reply if queried for confirmation that the Extension exists, like so:


window.addEventListener("message", function (event) {
  if (
    event.source == window &&
    event.data.message === "REQUEST_EXTENSION" &&
    event.data.source === "APP"
  ) {
    window.postMessage(
      {
        source: "EXT",
        message: "CONFIRM_EXTENSION",
      },
      "*"
    );
  }
});

This means if the APP sends a message with that information this code will create a new message that can be handled by the APP to do a certain function. With the app, the old fuction now creates a eventListener that runs the setHasExtension in case there is a confirm message. This only is sent if the extension as injected to code after being installed.

function checkExtensionInstalled(setHasExtension) {
  window.addEventListener("message", function (event) {
    if (
      event.source == window &&
      event.data.message === "CONFIRM_EXTENSION" &&
      event.data.source === "EXT"
    ) {
      setHasExtension(true);
    }
  });
  window.postMessage(
    {
      source: "APP",
      message: "REQUEST_EXTENSION",
    },
    "*",
  );
}

I have made all the hasExtension variables FALSE** by default, and are only set to true by this method.

NOTE: This is handled equality in Firefox and Chrome, however, it means that the user needs to refresh the page so that the code is injected in the webpage and the check can be made. I could add some logic to check the tabs and attempt to reload, but from what I understand when we install the APP we will be shown a page to re-open Zeeguu which would then execute the code.

mircealungu commented 6 months ago

Beautiful solution @tfnribeiro !

Maybe for readability:

mircealungu commented 6 months ago

I have made all the hasExtension variables FALSE** by default, and are only set to true by this method.

This is cool!

NOTE: This is handled equality in Firefox and Chrome, however, it means that the user needs to refresh the page so that the code is injected in the webpage and the check can be made. I could add some logic to check the tabs and attempt to reload, but from what I understand when we install the APP we will be shown a page to re-open Zeeguu which would then execute the code.

Indeed, the workflow I imagine will be something like:

So that should not require any installation. Indeed, if the user instead of clicking on the install extension would open a new tab, install the extension, and then later go back to the first tab, they will see the install the extension message. but that's rare, and all they have to do is to refresh, so it should be totally fine.

tfnribeiro commented 6 months ago

Sounds good!

I will test the workflow as you describe and make the PRs public and attach them in this issue so they are linked to the issue. I don't see any issue in that work flow, as after the extension is installed the code is injected anytime any zeeguu.org links are opened.

tfnribeiro commented 6 months ago

Firefox

  1. Opened the app and clicked on an Unsaved Article and got the notification
  2. Manually added the extension and got the "Go to Zeeguu App" (https://www.zeeguu.org/extension_installed)
  3. Re-clicked the article, now it showed the modal to go to the webpage and open using the context menu.

Chrome

  1. Opened the app, clicked on the first Unsaved Article got the error notification
  2. Added the temporary extension and reloaded the zeeguu.org/articles page.
  3. Saw the console.log of injecting the code, however this didn't remove the prompt message.
  4. Going to another tab, and returning fixed the issue.
  5. Adding a 100ms time out to running the checking the extension code fixed the issue.

This might be how chrome renders the page, where it might render part of it before the events are all added. Adding this small delay seems to have fixed it, but I am not sure what might cause it.

Browser Extension PR: https://github.com/zeeguu/browser-extension/pull/53 Web PR: https://github.com/zeeguu/web/pull/336