-
Notifications
You must be signed in to change notification settings - Fork 717
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
Add conditional check to ensure resource exists before rendering report page #11843
Add conditional check to ensure resource exists before rendering report page #11843
Conversation
Build Artifacts
|
I don't think so - this page shouldn't even be linked to in the case that the resource is completely missing (even metadata), so it doesn't seem like a problem. |
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.
This looks like it fixes it - did you try doing the conditionality inside the ReportsResourceHeader instead? That might be a more general fix, as I think that's where the missing resource flag is displayed.
Good idea - I will make that switch |
Sorry to say that the current EXE asset does not seem to solve the issue on the same Windows 7 VM where it was originally reported. I'm still seeing all the issues listed there. loading4C16GB-11843.mp4I installed the EXE asset from this PR, and imported the same facility from the original issue (home folder from this installation). @pcenov Could you re-test to confirm? Thank you! |
Yes I am able to replicate the issue on Chrome(Ubuntu) specifically when viewing the Difficult questions. It's not always happening, but can be reliably replicated if you manually refresh the page: 2024-02-08_11-26-33.mp4 |
9605a61
to
f1a6b03
Compare
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.
This is the right change to distinguish between the resource availability being false and just being falsy by dint of its absence.
Manual testing confirms the fix for the missing resource banner. The NaN B remains, but that seems like a lower priority issue that can be separated out to be addressed with other extant lower priority coach loading state issues. |
So this latest asset does not display the As mentioned, the loading4C16GB-11843b.mp4 |
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.
After speaking with @marcellamaki, we decided to merge this as-is, and I'll open separate issues for the remaining 2 loading states hick-ups.
Summary
Adds a check to ensure that
resource
which is necessary to render the page fully, exists in the state before it renders. This prevents the "missing resource" from appearing when the resource does exist.References
Fixes #11824
Reviewer guidance
Do I need to do some other sort of more robust error handling here in case the resource does not in fact exist on the device and missing resource alert is more appropriate @rtibbles ? (
coreLoading
does not work as a condition)Testing checklist
PR process
Reviewer checklist
yarn
andpip
)