Skip to content
This repository has been archived by the owner on Jul 23, 2020. It is now read-only.

Deployment charts can be rendered improperly if redirecting to a different page while charts are initializing #3110

Closed
aptmac opened this issue Apr 12, 2018 · 13 comments

Comments

@aptmac
Copy link
Collaborator

aptmac commented Apr 12, 2018

Occasionally, the deployment charts can be rendered in such a way that the chart lines and graphics don't show, but the data points & tooltips still work.

When on the deployments page, if another page is visited (e.g., codebases, pipelines) while the charts are being initialized (a good indicator of this is when the donut charts are white and haven't been filled in blue yet), then upon re-visiting the deployments page the charts will not render correctly. The following error message is shown in the console:

console-error

Example:
weird-sideeffect

Gifs:
Here's what it looked like one time when I encountered it:
process

And what the charts look/function like after:
blank-charts

@andrewazores
Copy link
Collaborator

This looks suspiciously similar to some c3 async loading stuff we've run into before, but I can't remember what the context for that was last time. @jiekang do you remember?

@jiekang
Copy link
Collaborator

jiekang commented Apr 16, 2018

Yes now that you mention it. I think we've looked at this before. c3js runs some timers async and doesn't clean things up properly, so this behavior is 'expected' for c3js.

@jiekang
Copy link
Collaborator

jiekang commented Apr 16, 2018

I wonder if I ever filed an issue for it.. Dang.

@aptmac
Copy link
Collaborator Author

aptmac commented Apr 16, 2018

Yeah it looks to be an async problem. The function where the null value is showing up is c3.convertDataToTargets() [0], which is being called after loading the new chart data at DeploymentsDonutChartComponent.updateChart() [1]. If I put a breakpoint at [1] (or a console log at [0], I haven't been able to put breakpoints into node_modules), I haven't been able to see a null value because it gets resolved by then, but c3 is seeing null.

fwiw if I replace [1] to regenerate the chart instead of loading like: this.chart = c3.generate(this.config); then I stop seeing the error, but it's not as nice a solution.

[0] https://github.com/c3js/c3/blob/master/c3.js#L5548
[1] https://github.com/fabric8-ui/fabric8-ui/blob/master/src/app/space/create/deployments/deployments-donut/deployments-donut-chart/deployments-donut-chart.component.ts#L147

@jiekang
Copy link
Collaborator

jiekang commented Apr 16, 2018

Ah found it:
fabric8-ui/fabric8-ui#2359

c3js/c3#1205
bcbcarl/react-c3js#22

See my comments in pull request, and there was a mitigation. I'm not sure if this is the same error as you reported, but I expect it to be similar.

@jiekang
Copy link
Collaborator

jiekang commented Apr 16, 2018

The mitigation isn't as good when the charts are in the process of loading, unfortunately. I'm not sure how much we can do on our side.

@aptmac
Copy link
Collaborator Author

aptmac commented Apr 16, 2018

Ah yes, it seems to be the same error as the c3 issue you've linked above.

@andrewazores
Copy link
Collaborator

I suspect part of the solution for this other issue (#3454) might solve or greatly mitigate this one as well.

@andrewazores
Copy link
Collaborator

Is this still reproducible?

@aptmac
Copy link
Collaborator Author

aptmac commented May 22, 2018

Yeah I was able to reproduce it just now - changing the page while the donut chart is transitioning from white to blue will cause the error still.

Edit: I was running a local build with all the latest changes

@andrewazores
Copy link
Collaborator

Okay, that's unfortunate.

The c3 bug we had seen was closed, but this recent and still open one looks the same: c3js/c3#2187

@stevengutz stevengutz added priority/P4 Normal and removed priority/P4 Normal labels Aug 2, 2018
@alexeykazakov
Copy link
Member

Outdated.

@aptmac
Copy link
Collaborator Author

aptmac commented Dec 4, 2018

This can still reproduced, but the bug is in the c3js repo. Even if/when it gets fixed in c3js it may be a headache to update patternfly to the latest version.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
OpenShift.io Plan
  
For PM Classification - don't mov...
Development

No branches or pull requests

6 participants