search

pulsar-edit / package-frontend

Frontend Site for Browsing Packages
https://web.pulsar-edit.dev/
MIT License
13 stars 8 forks source link

Fix site-wide 404 & Pass along HTTP status info to error handler #123

Closed confused-Techie closed 10 months ago

confused-Techie commented 10 months ago

With the brand new logging we have setup, I was able to find a bug occurring in prod. While the error handler expected an object, it was only being given a 404 status code number during the site wide 404 error. Which would cause it to crash.

This PR fixes this, ensuring that site wide 404's are caught properly by the generic error handler. Additionally, I've gone ahead and added some logic that lets other errors, such as those when displaying an individual package page, can bubble up the HTTP status code they received to the error handler, providing the HTTP error code the user will actually receive. Which can help make the logs easier to inspect, and keeps things much more accurate

DeeDeeG commented 10 months ago

Additionally, I've gone ahead and added some logic that lets other errors, such as those when displaying an individual package page, can bubble up the HTTP status code they received to the error handler, providing the HTTP error code the user will actually receive. Which can help make the logs easier to inspect, and keeps things much more accurate

Big 👍 on this! Reviewing the diff now...

Also, not crashing on 404, sounds like another big 👍 , lol.

DeeDeeG commented 10 months ago

Checking if the indentation was as deep as intended is pretty straight-forward, and the rest are style things about some logic checks that I don't feel strongly about.

Overall 👍 regardless of these comments. The approve above is valid for merging as-is, if you've considered the comments and determine that no changes are intended/desired.

confused-Techie commented 10 months ago

@DeeDeeG Thanks a ton for giving this one a review, lots of fantastic feedback, especially about shortening those logic checks. I'll go ahead and address those properly tomorrow, just giving a nod to show I've seen it, thanks!

confused-Techie commented 10 months ago

@DeeDeeG Thanks a ton for the feedback, sorry it took some time to get too.

I've gone ahead and addressed this feedback and updated the commit following all three of your well thought out suggestions. Thanks again and feel free to let me know what you think!