User/mattwar/large data charts#93
Conversation
…tab switch or resize
There was a problem hiding this comment.
Pull request overview
This PR improves Plotly chart responsiveness for large result sets in the VS Code results viewer by reducing DOM-heavy SVG rendering, avoiding expensive HTML/script reinjection, and making resizes/layout updates cheaper.
Changes:
- Switches large line/scatter/non-stacked-area traces to WebGL (
scattergl) above a point threshold. - Introduces structured single-chart rendering via
postMessagepayloads ({ divId, dataJson, layoutJson, configJson }) and faster JSON embedding (JSON.parse('...')) for HTML-based paths. - Optimizes resize behavior (uses
Plotly.relayout, wrapper positioning, rAF resize trigger) and skips sorting when x is already monotonic.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Client/tests/unit/plotlyChartProvider.test.ts | Updates tests for structured setChartContent rendering and adds coverage for scattergl threshold + monotonic-x fast path. |
| src/Client/features/resultsViewer.ts | Adjusts chart container CSS to prevent ResizeObserver loops and uses requestAnimationFrame for resize triggering on view/panel toggles. |
| src/Client/features/plotlyChartProvider.ts | Implements WebGL trace selection, structured single-chart payload rendering, faster JSON embedding, monotonic-x sort skip, and relayout-based resize updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Apply font/size overrides to the already-rendered plot via relayout. | ||
| // newPlot would force Plotly to re-upload all trace data and rebuild the | ||
| // GL canvas; relayout updates layout-only properties in place. | ||
| var overrides = Object.assign({ width: targetW, height: targetH }, applyFontOverrides(plotDiv.layout || {}, targetW, targetH, preset)); | ||
| Plotly.newPlot(plotDiv, plotDiv.data, Object.assign({}, plotDiv.layout, overrides), plotDiv._context); | ||
| Plotly.relayout(plotDiv, overrides); |
There was a problem hiding this comment.
_chartUpdated() still uses the non–aspect-ratio branch that clears wrapperDiv.style.position/left/top (see just above this block), while resizeChartCore() now relies on the wrapper being absolutely positioned in fill mode to prevent ResizeObserver feedback loops. Consider updating _chartUpdated() to apply the same fill-mode wrapper positioning (absolute + left/top = 0) so a post-update layout change doesn’t immediately retrigger an extra resize.
Co-authored-by: Copilot <copilot@github.com>
Speed up chart rendering and resize for large result sets
A 50,000-row chart used to take minutes and was unresponsive to resize/edit. After this PR, it renders in a couple of seconds and resizes cleanly.
Changes
Follow-up fixes from PR feedback
<script>is valid on any runtime._chartUpdatedtoo, so the initial render can't trigger the sameResizeObserverfeedback loop the resize path was already protected against.chartViewReadyhandshake sosetChartContentis reliably delivered on initial load and afterwebview.htmlrebuilds, with the latest payload cached inPlotlyChartViewfor replay.setChartContenthandler against untrustedpostMessagepayloads: validatedivIdagainst a strict pattern and build the placeholder element viacreateElement/appendChildinstead ofinnerHTMLconcatenation.decodeJsStringLiteralwith the productionescapeForJsStringLiteralso the structured-invoke path round-trips correctly for U+2028 / U+2029.Tests
All 112 unit tests pass; added cases for the scattergl threshold (and that stacked area stays on SVG), the monotonic-x fast path, and the new structured setChartContent invocation.