@MarkTraceur what's the current status of this product? Can I try it somewhere on Beta?

@Jseddon Let me know where we can test this

Not a performance bug, but FYI there is a vertical alignment issue when the browser window is not very wide and there is an image inside the infobox. The spinner is in the right spot, but then the graph appears below the infobox instead of next to it:

Here is my first pass of review. I might have missed small things, but there's already a lot to address.

There is no visual fallback when JS is disabled. I understand that the idea is to stop have server-side rendering for this, but there isn't even any text explaining that there's supposed to be a graph there and it couldn't be rendered due to lack of JS (or said JS being blocked). This information needs to be conveyed, particularly for users who may consider enabling JS or unblocking it to view the graph. I see that there's a recent commit by @Jseddon doing something like this that just not be live yet on testwiki.

The progress spinner appears even if it's only going to be visible for a split second. The current performance perception best practice is to only have spinner/progress bars display for processes that take a significant amount of time. Otherwise, the spinner brings attention to a process that may actually be fast, giving the false impression that it's going to take a while. Basically, don't draw attention to something that might be fast. The recommended fix is simple: just wait for 1 second (or maybe even more) before displaying the spinner. In a lot of cases, the graph will appear in less time than that short wait and the user won't have to view the spinner at all. It will look just like images loading on the page. In fact, if the goal is to make it look like an image (like it used to be), doing without the spinner altogether would make it consistent in visual loading behaviour.

The spinner is both embedded in CSS and referred to as a URL (/w/extensions/Graph/includes/ajax-loader.gif?b3af8). Just keep the URL one, which seems to be the one actually used. Remove the embedded copy from the CSS. Images embedded in CSS are now discouraged anyway.

There doesn't seem to be any instrumentation of the performance from the end-user's perspective. This can be a good way to find bugs. Eg. graphs that take a long time to render, or don't render at all. Simply use a mw.track call looking at the difference in monotonic time between when the DOM appears and when the graph has actually been rendered. You can then turn that into a Grafana dashboard very easily. And even set up alerts, which would let you detect major breakages that may happen when you update the extension later, for example.

The JS request oddly includes jQuery jQuery should already be present on any mediawiki pageview. Why does the extension load jquery again?

The JS is huge. 228kb compressed, 852kb to parse uncompressed. Without jquery it's still 184kb compressed/698kb uncompressed. Since the bulk of that is 3rd-party libraries, it might be useful to get a wide selection of graphs and do some usage check on the JS codebase. Just to get an idea of whether we're really using large portions of those libraries or not. It might also be worth checking if the newer versions of those libraries are leaner or more modular. The version of vega used (2.6.3) is 4 years old, current version is 5.13.0. The version of D3 used (3.5.17) is 2 years old, current version is 5.16.0.

Loading the JS and the data isn't parallelised. This is especially critical considering the sheer size of the JS. I see that there's a TODO about this in the code, but that this code is unused at the moment. It seems like vega2 is included with the page directly instead of using ext.graph.loader

The graph JSON isn't cached. The API call that gets it doesn't have any caching parameter. It should be trivial to add those with a maxage parameter. Maybe don't add caching if the user is logged-in so that editors who modify the graph data see their updates immediately, but for unlogged visitors, the graph data should be cached for at the very least a few minutes.

In T240626#6256072, @Gilles wrote:

Not a performance bug, but FYI there is a vertical alignment issue when the browser window is not very wide and there is an image inside the infobox. The spinner is in the right spot, but then the graph appears below the infobox instead of next to it:

Yeah I noticed this really unappealing behaiviour when creating the fallback option. The graph dimensions provided by a user is only an approximation of the final generated graph size.

This has been launched, but as far as I'm aware all the issues that I flagged in my review don't have tasks. I assume that none of the issues have been addressed either?