Feat/render errors in modal

[0x4007]

Resolves #50

• Also shows an error icon in the modal header to distinguish between normal behavior and error display behavior.
• Manually tested normal errors and rejected promises.
  • Note that you can't pass in an error in the console, as it runs in an isolated runtime apparently, and the window.onerror event does not capture it. Instead you need to add a throw new Error() inside the code for testing.

[ubiquity-os-deployer[bot]]

26b3121

26b3121

45cc922

e5e3511

b3b274c

9243a6e

88eafce

5c50b2c

[0x4007]

Reviewed the spec and realized I need a Cypress test. Working on figuring this out now.

[0x4007]

The last successful run of Cypress was three months ago with a single test, so it appears as though they were never stable in the first place @gentlementlegen

Perhaps the Cypress tests should all be fixed as part of a separate task.

[gentlementlegen]

@0x4007 Usual test cases works fine, although I do not get any modal on network failures, I simulated them an just got a blank screen, do not know if that's wanted. c.f. screenshot

Unrelated issue but users that do not have a name set have a blank display next to them, might better use the login field

---

Ran tests locally, the new test fails with error

AssertionError: Timed out retrying after 4000ms: expected '<div.preview-header>' to be 'visible'

This element `<div.preview-header>` is not visible because its parent `<div.preview>` has CSS property: `opacity: 0`

Will test locally by merging my fix branch and see if tests are resolved.

---

The new added test also fails in the other branch. Will fix after merge.

AssertionError: Timed out retrying after 4000ms: expected '<div.preview-header>' to contain 'Something went wrong'

[0x4007]

@0x4007 Usual test cases works fine, although I do not get any modal on network failures, I simulated them an just got a blank screen, do not know if that's wanted.

How exactly did you simulate? Any rejected promises within the code (not via the console/repl) throw the modal.

Unrelated issue but users that do not have a name set have a blank display next to them, might better use the login field

You can file a new issue.

Ran tests locally, the new test fails with error

AssertionError: Timed out retrying after 4000ms: expected '<div.preview-header>' to be 'visible'

This element `<div.preview-header>` is not visible because its parent `<div.preview>` has CSS property: `opacity: 0`

Tests don't work. This is expected.

[gentlementlegen]

@0x4007 I blocked the requests within the browser to make them fail. You can also simulate by turning the network "offline" so all the requests fail as well.

I will file a new issue!

All the tests work except yours, will fix in the other pull request.

[0x4007]

@0x4007 I blocked the requests within the browser to make them fail. You can also simulate by turning the network "offline" so all the requests fail as well.

Will check.

All the tests work except yours, will fix in the other pull request.

This sounds like an implicit approval. Can you merge?

[0x4007]

Just got it! The try/catches were catching and not letting some errors propagate to the top. I was able to replicate what you did by setting my network to "offline" while loading.

[gentlementlegen]

@0x4007 When accessing the page and being logged-out, I have this being displayed:

Is it intended?

[0x4007]

I believe that this is technically correct, because I interpret it as a bug with the cache.

I interpret as that you have some privileged access to some information when you are logged in, which is partially retained in the cache (we have two layers, the preview and the full details.)

When you are logged out, the cache is not reinitialized correctly, leading to this error.

These are my assumptions only based on my experience dealing with the cache problems some time ago when I first implemented it.

---

Running it locally I see these errors.

Based on the latest, I assume there is something in the cache that suggests you are logged in, and then it tries to request the current logged in user details. Because you are not currently logged in (I believe the endpoint is generic, literally like api.github.com/user, but includes your auth token in the headers) the endpoint says you aren't actually a real logged in user.

So in conclusion, any errors shown by the modal I am confident are real errors.

[0x4007]

Just stumbled upon this will require more investigation. Should have been caught by window.onerror

[gentlementlegen]

@0x4007 But if you simply access the website as a logged-out user, should this be displayed? I know that this call will fail for anyone logged-out / not part of the org. It's an expected error because Octokit throws with a 404. Maybe should be caught and not thrown?

[0x4007]

@0x4007 But if you simply access the website as a logged-out user, should this be displayed? I know that this call will fail for anyone logged-out / not part of the org. It's an expected error because Octokit throws with a 404. Maybe should be caught and not thrown?

We need to not make the network request unless the user is logged in. What I'm suggesting is that this is an actual error, and should be fixed as part of a separate task.

---

Off topic but:

Use window.onerror to catch unhandled errors globally, but it won’t catch errors from within async functions or certain other contexts.

So I'm going in and wrapping all of the other event handlers now with this modal.

[0x4007]

The theory is correct. Just need to wrap all of the event handlers. Almost finished...

[0x4007]

Good to go!

As a heads up, I especially handled the rate limiting and automatic logout problems at 9243a6ebc3031510930cfd874f474797b7b6a673 I don't have a screenshot of that unfortunately but I do have of the other error: