Skip to content
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

Merged
merged 13 commits into from
Jul 18, 2023

Conversation

JacobDiazCruz
Copy link
Contributor

@JacobDiazCruz JacobDiazCruz commented Dec 21, 2022

Closes #1618
Closes #2000
Closes #1809
Closes #1739

image

@agalin920
Copy link
Contributor

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:
In what scenarios can the model be empty?
Are we rendering the component with an empty model or are we rendering a loading view while it is being fetched?
What are the conditions for my loading view and does it account for model?

Also, remember to clear indexed db and/or throttle internet speed for more thorough debugging

Copy link
Contributor

@agalin920 agalin920 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolve comments

@JacobDiazCruz
Copy link
Contributor Author

JacobDiazCruz commented Jan 11, 2023

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.
But still, to answer your questions:

  1. In any case, model prop/object should always have values since content items with SEO & meta are always dependent on them. The only thing that I can’t see is the empty "type" value within the model object. Which leads me to just applying a default string in case the "type" is undefined.

  2. No, we're not rendering the component with an empty model. The model is already served when the parent Content component is rendered.

P.S I removed the empty object to avoid confusion.

@agalin920
Copy link
Contributor

agalin920 commented Jan 18, 2023

@JacobDiazCruz Can you share the model ZUID that has an empty type string?

  1. According to the API this should never happen and if its a malformed record we should bring it up. In addition what kind of effects would occur if we default to empty string type? A type that should not be accounted for

  2. Are you sure? What if the models are still fetching? Does it account for that?

@JacobDiazCruz
Copy link
Contributor Author

Unfortunately, I can't replicate this issue on dev and I added a comment regarding that on the bug ticket #1618.

  1. Upon checking again, the child components that contains this "type" prop is unused. So I can still safely perform actions and there are no effects if the type field is empty string.

  2. There is a loading state when the models are still fetching on the parent component, so the Action component won't render before that.

@agalin920
Copy link
Contributor

agalin920 commented Jan 30, 2023

@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

@JacobDiazCruz
Copy link
Contributor Author

Got it, I think its best to just remove this code.

@finnar-bin finnar-bin requested a review from agalin920 May 7, 2023 23:51
@finnar-bin finnar-bin self-assigned this May 7, 2023
@finnar-bin
Copy link
Contributor

finnar-bin commented May 7, 2023

Rendering the <NotFound /> component when props.model is empty also fixes the issue raised on #1739
Note: This flow already exists when viewing the list of items for headless and multi-page data types. I just went ahead and applied the same logic to single page. This also makes sure that no weird empty UI states are being rendered when prop.model is empty.

@finnar-bin finnar-bin changed the title Added empty string to avoid undefined type FIX: Add an error message screen when model fails to load May 8, 2023
@agalin920
Copy link
Contributor

@theofficialnar This does not seem right to me.

We already have handling for this so this is redundant.

Screen Shot 2023-05-07 at 8 13 32 PM

@finnar-bin
Copy link
Contributor

finnar-bin commented May 8, 2023

@theofficialnar This does not seem right to me.

We already have handling for this so this is redundant.

Screen Shot 2023-05-07 at 8 13 32 PM

@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?

@agalin920
Copy link
Contributor

agalin920 commented May 8, 2023

@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

@agalin920
Copy link
Contributor

agalin920 commented May 8, 2023

@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

@finnar-bin
Copy link
Contributor

@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.

@agalin920
Copy link
Contributor

@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?

Copy link
Contributor

@agalin920 agalin920 left a 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?

@finnar-bin
Copy link
Contributor

@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.

@finnar-bin finnar-bin requested a review from agalin920 May 8, 2023 06:23
@finnar-bin
Copy link
Contributor

Added a more simpler fix @agalin920

@shrunyan shrunyan merged commit ff55958 into master Jul 18, 2023
@shrunyan shrunyan deleted the bugfix/undefined-type-key-in-content-editor branch July 18, 2023 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment