[UI] Fix bug where UI endlessly spins on attempting to view a metric from the runs page #1978
Conversation
…he RequestStateWrapper had 0 requests
smurching
added
the
area/uiux
| @@ -34,9 +34,9 @@ export class RequestStateWrapper extends Component { | |||
| } | |||
|
|
|||
| static getDerivedStateFromProps(nextProps) { | |||
| const shouldRender = nextProps.requests.length ? nextProps.requests.every((r) => { | |||
| const shouldRender = nextProps.requests.every((r) => { | |||
There was a problem hiding this comment.
Adding the check of nextProps.requests.length is to support initial loading from componentDidMount. This will likely break model registry pages which make initial loads in component did mount. We should probably try using shouldOptimisticallyRender on the special case?
There was a problem hiding this comment.
@Zangr thanks for the comment! I'm not entirely sure I follow as the model registry components all seem to pass non-empty arrays of request IDs to RequestStateWrapper, (i.e. the empty length case never applies to them), and it seems if there are no requests passed to RequestStateWrapper, we should not spin endlessly. But using shouldOptimisticallyRender here does work, so I'll switch to that instead :)
There was a problem hiding this comment.
As React triggers componentDidMount in "post order traversal", the initial render of RequestStateWrapper will not find any requests. However[].every(...) returns true when the base array is empty. That will make the component show before data is ready, which can cause a flash of semi-ready content or NPE.
smurching
added
rn/none
…from the runs page (mlflow#1978) Fixes bug (which never shipped to users) introduced by mlflow#1943 that broke the metrics page
What changes are proposed in this pull request?
Fix bug in
MetricPlotPanelcomponent, which uses RequestStateWrapper with the intention of rendering a spinner if the user selects additional metrics to view/compare after the initial page load (i.e. makes additional metric history requests after the initial page load). However, on the initial page load there are no such additional metric history requests, so the Spinner continues to display. We mitigate this by passing theshouldOptimisticallyRenderprop so that the panel always renders (including in this case).How is this patch tested?
Manual testing of existing pages + added a JS regression test
Release Notes
Is this a user-facing change?
Fixes bug (which never shipped to users) introduced by #1943 that broke the metrics page
What component(s) does this PR affect?
How should the PR be classified in the release notes? Choose one:
rn/breaking-change- The PR will be mentioned in the "Breaking Changes" sectionrn/none- No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" sectionrn/feature- A new user-facing feature worth mentioning in the release notesrn/bug-fix- A user-facing bug fix worth mentioning in the release notesrn/documentation- A user-facing documentation change worth mentioning in the release notes