Feat/render errors in modal #57
Conversation
|
Reviewed the spec and realized I need a Cypress test. Working on figuring this out now. |
|
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. |
|
@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 Ran tests locally, the new test fails with error 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. |
How exactly did you simulate? Any rejected promises within the code (not via the console/repl) throw the modal.
You can file a new issue.
Tests don't work. This is expected. |
|
@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. |
Will check.
This sounds like an implicit approval. Can you merge? |
|
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.
|
|
@0x4007 When accessing the page and being logged-out, I have this being displayed: 
Is it intended? |
|
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. |
|
Just stumbled upon this will require more investigation. Should have been caught by 
|
|
@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: So I'm going in and wrapping all of the other event handlers now with this modal. |
|
The theory is correct. Just need to wrap all of the event handlers. Almost finished...
|
|
Good to go! As a heads up, I especially handled the rate limiting and automatic logout problems at 9243a6e I don't have a screenshot of that unfortunately but I do have of the other error:
|
| const reset = core.data.resources.core.reset; | ||
| // Mark private issues | ||
| const privateIssuesWithFlag = privateIssues.map((issue) => { | ||
| return issue; |
There was a problem hiding this comment.
Just realized this is unnecessary in its current form.
Resolves #50
window.onerrorevent does not capture it. Instead you need to add athrow new Error()inside the code for testing.