feat: add connection-error component and account error state #2530

lujakob · 2023-07-03T12:38:38Z

Describe the changes you have made in this PR

Add AccountMenu account error state. Add ConnectionError component.

Link this PR to an issue [optional]

Fixes #2377

Type of change

feat: New feature (non-breaking change which adds functionality)

Screenshots of the changes [optional]

Add screenshots to make your changes easier to understand. You can also add a video here.

How has this been tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Checklist

lujakob · 2023-07-03T12:39:33Z

@reneaaron this is a working draft. Refinement (especially in styles) is needed, but you could check out my changes/architecture if you want. I broke the "/v1/getinfoo" GET request for the LND connector to simulate failing connections.

github-actions · 2023-07-03T12:45:01Z

🚀 Thanks for the pull request!

Here are the current build files for testing:

Download and unzip the file for your browser. Refer to the readme for detailed install instructions.

Don't forget: keep earning sats!

reneaaron

Thanks for the PR @lujakob! Left some first feedback, generally I think we are on the right track here. 👍

reneaaron · 2023-07-03T14:32:44Z

src/common/lib/api.ts

@@ -70,26 +71,37 @@ export const swrGetAccountInfo = async (
  const accountsCache = await getAccountsCache();

  return new Promise((resolve, reject) => {
-    if (accountsCache[id]) {
+    // don't apply cache for connectors that failed to connect
+    if (accountsCache[id] && !accountsCache[id].error) {


Can we keep accounts with errors out of the cache? Why would we cache them if we exclude them from the cache here? 🤔

I changed it.

reneaaron · 2023-07-03T14:33:47Z

src/common/lib/api.ts

-        };
+        let account: AccountInfo;
+        if (response.error) {
+          account = {


Shouldn't this case be handled in the catch() block here?

msg.request should throw an Error():

https://github.com/lujakob/lightning-browser-extension/blob/feat/not-reachable-error/src/common/lib/msg.ts#L19-L21

Your suggestion was my first approach. The problem is, the argument of throw new Error() is of type string. But I need to pass an object, to be able to get the name of the connector and error. To change this, I need to refactor dozens of places where msg.request is called in a try/catch block. So on the one hand this is really a big refactor, on the other hand I think what we do now is not completely wrong: we return a connector in error state, that has name as its only property. Considering these two points I opted for the current approach. (Maybe it doesn't feel so wrong for me, because I used this approach in other applications, where the error returns data and therefore is not dealt with as a real error) What do you think?

reneaaron · 2023-07-03T14:42:58Z

src/app/router/Popup/Popup.tsx

+  const { account } = useAccount();
+  // keep last tries result, to prevent component flick during loading state
+  const [lastAccountFailed, setLastAccountFailed] = useState<boolean>(false);
+
+  useEffect(() => {
+    // skip the state during loading phase where only the id is present
+    if (Object.prototype.hasOwnProperty.call(account, "name")) {
+      setLastAccountFailed(!!account?.error);
+    }
+  }, [setLastAccountFailed, account]);
+


That feels a bit complex on the first sight. What is it that you want to prevent here?

Isn't it natural to go from error -> loading -> success / error?

You can disable it and see it. If you replace
{lastAccountFailed || account?.error ? <ConnectionError /> : <Outlet />} with
{account?.error ? <ConnectionError /> : <Outlet />} you see during the loading state after clicking on "Try again" you see the Outlet component for a short moment. A glitch...

lujakob · 2023-07-03T15:41:16Z

src/app/components/Button/index.tsx

+          <div
+            className={classNames(
+              direction === "row" ? "mr-2 flex justify-center min-w-[24px]" : ""
+            )}


@reneaaron this change in styles is necessary, because having an icon and loading state results in a very small horizontal jump of the text, due to the rotating loading icon and the icon itself not having the same width. I'm kind of unsatisfied with this change, but I found no better way in preventing this rather annoying UX problem. Please have a look and let me know. You can revert by "show diff" branch master and revert the changes to see what I mean and then test clicking "Try again" button to see the jump.

lujakob · 2023-07-13T17:30:14Z

@reneaaron I updated with master, abstracted the "connectionFailed" state to a hook function and added the connection error to the options page. I added extensive comments to the hook function for explanation. I think this is 95% done now, so let me know what you think. The LND getInfo request url is erroneous for testing. If you throttle/delay the network requests, you can see the loading spinner on the button and test the screen not flicking back to the normal content during loading.

rolznz · 2023-09-04T03:51:10Z

We currently have an issue where if you're unable to connect to a connector, then it's impossible to use LNURL auth there. I think it would it make sense to be able to still switch connector, but it be in a failed state (like this). And you can still use the LNURL auth and master key features. What do you think? @lujakob @reneaaron @bumi

reneaaron · 2023-09-04T07:04:30Z

Yeah, but that should be the case already, right? The failed state would only be reflected on the wallet page (but not in the prompts), right?

rolznz · 2023-09-04T08:17:20Z

Yeah, but that should be the case already, right? The failed state would only be reflected on the wallet page (but not in the prompts), right?

I don't think this has been implemented yet. I can't see any change to how the connector init works so that it will still return the connector if it fails

…ng-browser-extension into feat/not-reachable-error

lujakob · 2023-09-30T11:55:20Z

Yeah, but that should be the case already, right? The failed state would only be reflected on the wallet page (but not in the prompts), right?

I don't think this has been implemented yet. I can't see any change to how the connector init works so that it will still return the connector if it fails

@rolznz do you think it makes sense for me to look into this? I would need some more information as I don't have the full picture here.

My understanding of lnurl auth is it's a single sign on to log into websites. If I am on www.getalby.com and "login with lightning" the popup opens. Is this the whole thing or is there more to know? Why is there a path for the lnurl auth page in the Options.tsx?

By the way, how do you debug/test lnurl auth in dev mode? If I can successfully login to my www.getalby.com account with the production extension, but not with the dev mode extension.

Maybe a short chat would be helpful?

feat: add connection-error component and account error state

c3cfa70

reneaaron reviewed Jul 3, 2023

View reviewed changes

lujakob added 2 commits July 3, 2023 17:24

feat: getAccountInfo omit caching errored account

6f6e568

feat: add button loading min-width

81579e2

lujakob commented Jul 3, 2023

View reviewed changes

lujakob added 8 commits July 3, 2023 17:54

fix: connection-error component button loading

7e05ba9

feat: connection-error dark-mode styles

f4fac4c

fix: account-menu error icons

dd31c2d

feat: connection error colors

1ac2260

chore: merge master

059fd39

feat: connection error reload skip cache

5db23fe

feat: connection error for options; abstract connectionFailed

2272142

chore: merge master

e6e524e

lujakob marked this pull request as ready for review July 13, 2023 17:27

lujakob and others added 3 commits July 18, 2023 14:24

Merge branch 'master' into feat/not-reachable-error

ccee5af

Merge branch 'master' into feat/not-reachable-error

d7bb80f

Merge branch 'master' into feat/not-reachable-error

43cbdd8

rolznz self-assigned this Sep 4, 2023

lujakob added 2 commits September 30, 2023 13:27

chore: merge master

255a994

Merge branch 'feat/not-reachable-error' of github.com:lujakob/lightni…

14eb444

…ng-browser-extension into feat/not-reachable-error

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add connection-error component and account error state #2530

feat: add connection-error component and account error state #2530

lujakob commented Jul 3, 2023

lujakob commented Jul 3, 2023 •

edited

Loading

github-actions bot commented Jul 3, 2023 •

edited

Loading

reneaaron left a comment

reneaaron Jul 3, 2023

lujakob Jul 3, 2023

reneaaron Jul 3, 2023

lujakob Jul 3, 2023

reneaaron Jul 3, 2023

lujakob Jul 3, 2023

lujakob Jul 3, 2023 •

edited

Loading

lujakob commented Jul 13, 2023 •

edited

Loading

rolznz commented Sep 4, 2023

reneaaron commented Sep 4, 2023

rolznz commented Sep 4, 2023

lujakob commented Sep 30, 2023

feat: add connection-error component and account error state #2530

Are you sure you want to change the base?

feat: add connection-error component and account error state #2530

Conversation

lujakob commented Jul 3, 2023

Describe the changes you have made in this PR

Link this PR to an issue [optional]

Type of change

Screenshots of the changes [optional]

How has this been tested?

Checklist

lujakob commented Jul 3, 2023 • edited Loading

github-actions bot commented Jul 3, 2023 • edited Loading

reneaaron left a comment

Choose a reason for hiding this comment

reneaaron Jul 3, 2023

Choose a reason for hiding this comment

lujakob Jul 3, 2023

Choose a reason for hiding this comment

reneaaron Jul 3, 2023

Choose a reason for hiding this comment

lujakob Jul 3, 2023

Choose a reason for hiding this comment

reneaaron Jul 3, 2023

Choose a reason for hiding this comment

lujakob Jul 3, 2023

Choose a reason for hiding this comment

lujakob Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

lujakob commented Jul 13, 2023 • edited Loading

rolznz commented Sep 4, 2023

reneaaron commented Sep 4, 2023

rolznz commented Sep 4, 2023

lujakob commented Sep 30, 2023

lujakob commented Jul 3, 2023 •

edited

Loading

github-actions bot commented Jul 3, 2023 •

edited

Loading

lujakob Jul 3, 2023 •

edited

Loading

lujakob commented Jul 13, 2023 •

edited

Loading