-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIX: Add an error message screen when model fails to load #1655
FIX: Add an error message screen when model fails to load #1655
Conversation
This is not a satisfactory solution in my eyes. Looking at the sentry bug it looks like multiple things can fail if model is not populated. Furthermore having that model loaded is paramount for the Content route to render and behave properly. So the questions to ask that would lead to a more appropriate solution are: Also, remember to clear indexed db and/or throttle internet speed for more thorough debugging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolve comments
Upon checking my solution again, I think it's wrong that I've put an empty object in case the model is empty, since the model's value is already populated even before reaching this Action component.
P.S I removed the empty object to avoid confusion. |
@JacobDiazCruz Can you share the model ZUID that has an empty type string?
|
Unfortunately, I can't replicate this issue on dev and I added a comment regarding that on the bug ticket #1618.
|
@JacobDiazCruz I can replicate this by blocking the request to the models endpoint and clearing indexedDB. Can you share with me the piece of code that prevents the Action component to render without model data? Either way if it is not being used lets simply remove it then |
Got it, I think its best to just remove this code. |
Rendering the |
@agalin920 I originally wanted to use that component but the error message comes off to me like the item is missing instead of the item just failing to load. Wouldn't that confuse the users making them think that an item has been deleted? |
@theofficialnar What i mean is. I modified the URL to an invalid model zuid. Therefore the 'failure to load model' is already being handled so im not sure what you are attempting to solve with this change |
@theofficialnar The existing handling might be lacking so if anything I expect the current one to be updated as opposed to adding a nested child handling |
@agalin920, yeah I guess I could just add that handling to show the error message you mentioned when the model is undefined. I checked how that message is being shown and currently it only shows up when the api call to fetch the model item failed. |
@theofficialnar Also look into is why could a model be undefined while the item loads properly. Could the child component that actually uses the model data simply be attempting a render before fetching models completes? Is there a guard rail that makes sure models are loaded before the item with the component that uses model data? What model data is even needed? Can it be obtained from the URL instead? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is going off in a wrong direction. Lets rewind a bit. What was wrong with the previous approved solution?
@agalin920, the original fix resolves the issue with the missing modelType which wasn't really used anyway. But another error occurs which also breaks the app just like before this was fixed pertaining to a missing model ZUID. Actually, I just realized just now that I can just use the model zuid that's on the url param. |
Added a more simpler fix @agalin920 |
Closes #1618
Closes #2000
Closes #1809
Closes #1739