🖼️ Fixed #18973 - Added error state renderer to show a different message if 500 is returned#18977
🖼️ Fixed #18973 - Added error state renderer to show a different message if 500 is returned#18977snipe wants to merge 3 commits into
Conversation
… if 500 is returned
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
uberbrady
left a comment
There was a problem hiding this comment.
This is going to be great! We’ve definitely had issues in the past with customers and open-source users “freaking out” because they had “no records” - when in reality they just forgot to run migrations during an upgrade.
But one very big concern I have is the number of retries - 80 seems awfully high. If we did have a bug somewhere, and accidentally rolled it out into production (not super-likely, but not impossible), we would absolutely melt our servers with that number of retries I fear.
I’m not sure of the exact number of retries we should be attempting, but I’m thinking not much more than single-digits. Happy to hash that out with you to figure out exactly how many we want to do.
|
I want to take a swing at cleaning this up a little, it feels a little bit all-over-the-place. But if this, how it is, is how it has to be, I'll at least want to inject a bunch of comments explaining why we're having to do these things in all of these different places. |
|
Totally fair. The BS tables code is pretty crazy, because we have a bunch of things that have to happen in a certain order, and then sometimes have to re-render afterwards (think the tooltips, etc). If you can find a cleaner way to do this, I'm 100% on board and will not be the least bit salty. I hate this code too, but everything else I tried sucked worse. |
|
ALSO - if you do take a stab at this, remember that you have to test a buttload of other things to make sure they don't break, and they're all manual tests, since the UI testing we have won't be able to render the BS table space like a normal browser would. (We see this in the pa11y testing too - no matter how long I set the delay, the screenshots are always "table loading") Off the top of my head:
I already tested those things with my version, so you'll have to make sure nothing you do breaks that functionality. |
This shows a specific error if the API is returning a 500, versus just "no results". I've tested this on initial page load (I changed the users API index query to query for
users.gaddressinstead ofusers.addressto force a 500 response) and also search and advanced search.Fixes #18973