acdh-oeaw / vicav-vue3

Reimplementation of the VICAV Frontend based on Vue.js 3
https://vicav-vue.acdh-ch-dev.oeaw.ac.at
MIT License
0 stars 0 forks source link

feat: create global error page #102 #103

Closed ttechnicus closed 7 months ago

ttechnicus commented 8 months ago

Created global error page

simar0at commented 8 months ago

This looks good but it has to display the actual return code and the api-problem object if any. Also as discussed with @ttechnicus : Although semantically this should be a 500 error page, Kubernetes will kill a service returning 500 on / and not accept it as an update to a running deployment. So we need this to be 200 (or a redirect 300). My approach is to display as much debug related infos as nice as possible so the user is not scared but the developer can get create a fix fast.

ttechnicus commented 8 months ago

@simar0at "as much debug related infos as nice as possible so the user is not scared but the developer can get create a fix fast" – This is rather subjective. Please be more specific. In my understanding, one guid that identifies the error in the server logs should be enough information – if there is one.

simar0at commented 8 months ago

Correct, the last comment is not specific enough.
So I like the overall layout as it is right now (nice for the user) but:

ttechnicus commented 8 months ago
  • You have to include the error data that is availabel as a prop to error.vue and make it visible on a click on some heading or button or...

This is done by now.

  • This kind of error reporting will not work on Kubernetes. Kubernetese checks will see the 500 and restart the frontend service. This is only correct if the frontend service itself failed (e. g. due to an error in the nuxt program)

  • If the backend fails right now this just makes the frontend fail fataly and we need to fix that:

There is a difference between errors during server-side rendering and client-side running. That means, errors may be treated differently when the user loads the page, and when the page is already set up and running. The current issue is that even though the error handlers are there, and they are called (!), during SSR ("bootup") the error continues to propagate to the top level, and causes the app to crash and display the error page. You can test it by changing the api baseUrl to something invalid – the error page shows up with the message "fetch failed" despite it has been handled.

On the client side (once the application has successfully set up) the toasters already work as a means of notification in case of fetch errors.

  • Catch any 400 or 500 error in the communication with the backend and render an error page that has a normal 200 status code
  • Display the status code that came from the backend. Maybe there is something useful in the body of a nginx 502 or 503 error message but I don't know that yet. If there is more useful information as JSON make it visible on a click on some heading or button or...
  • If there is a api problem JSON in the body render that and make it visible on a click on some heading or button or...

Once the propagation issue is solved, we may see if we can enrich the error message with these data. This may require re-throwing the error with the necessary data added to the data field of the error object.

ttechnicus commented 8 months ago

Follow-up questions for further discussion after a meeting with @simar0at :

simar0at commented 7 months ago

Tried it locally, works as expected. @ctot-nondef, you are listed as the reviewer so I did not merge it.

The only thing that I am missing is a collapsed rendering of the error JSON that the server provides:

{
    "trace": "Q:\/BasexVicav\/webapp\/vicav-app\/vicav.xqm, 124\/18\r\n- Q:\/BasexVicav\/webapp\/vicav-app\/vicav.xqm, 106\/28\r\n- Q:\/BasexVicav\/webapp\/vicav-app\/api-problem.xqm, 41\/26",
    "detail": "test",
    "status": "500",
    "instance": "http:\/\/acdh.oeaw.ac.at\/vicav\/testerror",
    "took": 57.06,
    "title": "testerror: testerror",
    "type": "http:\/\/acdh.oeaw.ac.at\/vicav"
}

We should probably move this to a new issue.

ttechnicus commented 7 months ago

@simar0at It is technically non-trivial to pass the entire json to the error page, as nuxt does not provide it by itself (see). It is worth creating a separate task for that.

ctot-nondef commented 7 months ago

thanks everyone!

ttechnicus commented 7 months ago

@simar0at Created #113 for the json