From 58f86915302c97c23da1573effccbe02fb00a692 Mon Sep 17 00:00:00 2001 From: Matt Warren Date: Tue, 28 Apr 2026 14:05:29 -0700 Subject: [PATCH 1/8] Use scattergl with large data sets --- src/Client/features/plotlyChartProvider.ts | 26 ++++++++-- .../tests/unit/plotlyChartProvider.test.ts | 47 +++++++++++++++++++ 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/src/Client/features/plotlyChartProvider.ts b/src/Client/features/plotlyChartProvider.ts index b7fbbe7..49af1b8 100644 --- a/src/Client/features/plotlyChartProvider.ts +++ b/src/Client/features/plotlyChartProvider.ts @@ -34,6 +34,22 @@ const PlotlyScatterModes = { LinesAndMarkers: 'lines+markers', } as const; +/** + * Per-trace point count above which we switch from Plotly's SVG `scatter` trace + * to its WebGL `scattergl` trace. SVG renders one DOM node per marker/segment, + * so it becomes unresponsive in the low thousands of points; `scattergl` draws + * the whole trace into a single canvas and stays interactive into the millions. + * + * `scattergl` does NOT support `stackgroup`/`groupnorm`, so stacked area charts + * always stay on `scatter` regardless of point count. + */ +const WebGlPointThreshold = 1000; + +/** Returns 'scattergl' when the trace exceeds the WebGL threshold and is GL-compatible, else 'scatter'. */ +function pickScatterType(pointCount: number, glCompatible = true): 'scatter' | 'scattergl' { + return glCompatible && pointCount > WebGlPointThreshold ? 'scattergl' : 'scatter'; +} + const PlotlyFillModes = { ToZeroY: 'tozeroy', ToNextY: 'tonexty', @@ -284,7 +300,7 @@ interface BarTrace extends PlotlyTraceBase { } interface ScatterTrace extends PlotlyTraceBase { - type: 'scatter'; + type: 'scatter' | 'scattergl'; x: unknown[]; y: unknown[]; mode: string; @@ -601,7 +617,7 @@ class PlotlyChartBuilder { else if (showMarkers) mode = PlotlyScatterModes.LinesAndMarkers; else if (showValues) mode = 'lines+text'; const trace: ScatterTrace = { - type: 'scatter', + type: pickScatterType(x.length), x, y, mode, @@ -627,7 +643,7 @@ class PlotlyChartBuilder { ): PlotlyChartBuilder { const hasMarker = markerSymbol != null || markerSize != null; const trace: ScatterTrace = { - type: 'scatter', + type: pickScatterType(x.length), x, y, mode: showValues ? 'markers+text' : PlotlyScatterModes.Markers, @@ -663,8 +679,10 @@ class PlotlyChartBuilder { if (showMarkers && showValues) mode = 'lines+markers+text'; else if (showMarkers) mode = PlotlyScatterModes.LinesAndMarkers; else if (showValues) mode = 'lines+text'; + // scattergl does not support stackgroup/groupnorm, so stacked area must stay on SVG scatter. + const glCompatible = stackGroup == null && groupNorm == null; const trace: ScatterTrace = { - type: 'scatter', + type: pickScatterType(x.length, glCompatible), x, y, mode, diff --git a/src/Client/tests/unit/plotlyChartProvider.test.ts b/src/Client/tests/unit/plotlyChartProvider.test.ts index b8c7d95..7ef64b7 100644 --- a/src/Client/tests/unit/plotlyChartProvider.test.ts +++ b/src/Client/tests/unit/plotlyChartProvider.test.ts @@ -445,6 +445,53 @@ describe('CompositeChartProvider', () => { }); }); + describe('scattergl threshold', () => { + // Generates a 2-column (datetime, real) table with `n` rows. + function makeLargeTimeSeries(n: number): ResultTable { + const rows: unknown[][] = new Array(n); + const base = new Date('2025-01-01T00:00:00Z').getTime(); + for (let i = 0; i < n; i++) { + rows[i] = [new Date(base + i * 60_000).toISOString(), i]; + } + return makeTable( + [{ name: 'Time', type: 'datetime' }, { name: 'Value', type: 'real' }], + rows, + ); + } + + it('keeps SVG scatter for line charts at or below threshold', () => { + const html = renderAndGetHtml(makeLargeTimeSeries(1000), { type: 'Line' }); + const trace = parseTraces(html!)[0] as Record; + expect(trace.type).toBe('scatter'); + }); + + it('switches line charts above threshold to scattergl', () => { + const html = renderAndGetHtml(makeLargeTimeSeries(1001), { type: 'Line' }); + const trace = parseTraces(html!)[0] as Record; + expect(trace.type).toBe('scattergl'); + }); + + it('switches scatter charts above threshold to scattergl', () => { + const html = renderAndGetHtml(makeLargeTimeSeries(2000), { type: 'Scatter' }); + const trace = parseTraces(html!)[0] as Record; + expect(trace.type).toBe('scattergl'); + }); + + it('switches non-stacked area charts above threshold to scattergl', () => { + const html = renderAndGetHtml(makeLargeTimeSeries(2000), { type: 'Area' }); + const trace = parseTraces(html!)[0] as Record; + expect(trace.type).toBe('scattergl'); + expect(trace.fill).toBe('tozeroy'); + }); + + it('keeps stacked area charts on SVG scatter regardless of size (stackgroup is unsupported in scattergl)', () => { + const html = renderAndGetHtml(makeLargeTimeSeries(5000), { type: 'AreaStacked' }); + const trace = parseTraces(html!)[0] as Record; + expect(trace.type).toBe('scatter'); + expect(trace.stackgroup).toBe('1'); + }); + }); + describe('piechart', () => { it('renders a pie trace with labels and values', () => { const html = renderAndGetHtml(makePieTable(), { type: 'Pie' }); From b84572a52c44643ae8a0aedfa70ee691370fd34f Mon Sep 17 00:00:00 2001 From: Matt Warren Date: Tue, 28 Apr 2026 14:19:18 -0700 Subject: [PATCH 2/8] Encode data as string instead of direct json in html to improve perf. --- src/Client/features/plotlyChartProvider.ts | 29 +++++++++++++++++-- .../tests/unit/plotlyChartProvider.test.ts | 25 ++++++++++++---- 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/src/Client/features/plotlyChartProvider.ts b/src/Client/features/plotlyChartProvider.ts index 49af1b8..f1259a3 100644 --- a/src/Client/features/plotlyChartProvider.ts +++ b/src/Client/features/plotlyChartProvider.ts @@ -972,13 +972,36 @@ class PlotlyChartBuilder { const PlotlyJsCdn = 'https://cdn.plot.ly/plotly-2.27.0.min.js'; +/** + * Escapes a JSON string so it can be embedded inside a single-quoted JS string + * literal and recovered with `JSON.parse`. We escape: + * - `\\` (must come first) so JSON's own escapes survive JS-string decoding, + * - `'` so we don't terminate the surrounding JS string, + * - `` in the data can't break out of the + * enclosing ``; diff --git a/src/Client/tests/unit/plotlyChartProvider.test.ts b/src/Client/tests/unit/plotlyChartProvider.test.ts index 7ef64b7..8217966 100644 --- a/src/Client/tests/unit/plotlyChartProvider.test.ts +++ b/src/Client/tests/unit/plotlyChartProvider.test.ts @@ -37,6 +37,19 @@ function createMockWebView(): IWebView & { // ─── Test Data Helpers ────────────────────────────────────────────────────── +/** + * Reverses the encoding done by `escapeForJsStringLiteral` in plotlyChartProvider.ts + * so tests can pull the JSON payload out of the rendered `JSON.parse('...')` literal. + * Order matters: undo the backslash escape first, then the apostrophe escape, then + * the ` { view.renderChart(emptyTable, { type: 'Column' }, false); expect(webview.setContent).toHaveBeenCalledOnce(); const html = webview.setContent.mock.calls[0]![0] as string; - const traces = html.match(/var data = (\[[\s\S]*?\]);\s*var layout/); + const traces = html.match(/var data = JSON\.parse\('([\s\S]*?)'\);\s*var layout/); expect(traces).toBeTruthy(); - const parsed = JSON.parse(traces![1]!) as { x: unknown[]; y: unknown[] }[]; + const parsed = JSON.parse(decodeJsStringLiteral(traces![1]!)) as { x: unknown[]; y: unknown[] }[]; expect(parsed[0]!.x).toEqual([]); expect(parsed[0]!.y).toEqual([]); }); @@ -238,16 +251,16 @@ describe('CompositeChartProvider', () => { /** Helper to parse the Plotly data array from the rendered HTML. */ function parseTraces(html: string): unknown[] { - const dataMatch = html.match(/var data = (\[[\s\S]*?\]);\s*var layout/); + const dataMatch = html.match(/var data = JSON\.parse\('([\s\S]*?)'\);\s*var layout/); expect(dataMatch).toBeTruthy(); - return JSON.parse(dataMatch![1]!) as unknown[]; + return JSON.parse(decodeJsStringLiteral(dataMatch![1]!)) as unknown[]; } /** Helper to parse the Plotly layout from the rendered HTML. */ function parseLayout(html: string): Record { - const layoutMatch = html.match(/var layout = (\{[\s\S]*?\});\s*var config/); + const layoutMatch = html.match(/var layout = JSON\.parse\('([\s\S]*?)'\);\s*var config/); expect(layoutMatch).toBeTruthy(); - return JSON.parse(layoutMatch![1]!) as Record; + return JSON.parse(decodeJsStringLiteral(layoutMatch![1]!)) as Record; } describe('columnchart', () => { From 6b14ffe6b9927cfc6bb90fcd9f3eb91dc078caa1 Mon Sep 17 00:00:00 2001 From: Matt Warren Date: Tue, 28 Apr 2026 14:33:15 -0700 Subject: [PATCH 3/8] Do not sort already monotonic data --- src/Client/features/plotlyChartProvider.ts | 44 ++++++++++++---- .../tests/unit/plotlyChartProvider.test.ts | 52 +++++++++++++++++++ 2 files changed, 87 insertions(+), 9 deletions(-) diff --git a/src/Client/features/plotlyChartProvider.ts b/src/Client/features/plotlyChartProvider.ts index f1259a3..af5d0ed 100644 --- a/src/Client/features/plotlyChartProvider.ts +++ b/src/Client/features/plotlyChartProvider.ts @@ -1171,6 +1171,35 @@ function aggregateValues(values: number[], aggregation: AggregationType): number } } +/** + * Returns x/y sorted ascending by x. Skips all sorting work — and avoids + * allocating two new arrays — when the input is already monotonically + * non-decreasing in x, which is the common case for time-series data + * (Kusto queries that end in `order by t asc`, `range`-generated rows, + * binned aggregates emitted in bin order, etc.). + * + * The comparison uses raw JS `<`/`>`, which match the previous behavior + * for numbers, Date objects, and ISO-8601 datetime strings. + */ +function sortPointsByX(x: unknown[], y: number[]): { x: unknown[]; y: number[] } { + let sorted = true; + for (let i = 1; i < x.length; i++) { + if (x[i - 1]! > x[i]!) { sorted = false; break; } + } + if (sorted) return { x, y }; + const indices = new Array(x.length); + for (let i = 0; i < x.length; i++) indices[i] = i; + indices.sort((a, b) => (x[a]! < x[b]! ? -1 : x[a]! > x[b]! ? 1 : 0)); + const sortedX = new Array(x.length); + const sortedY = new Array(x.length); + for (let i = 0; i < x.length; i++) { + const j = indices[i]!; + sortedX[i] = x[j]; + sortedY[i] = y[j]!; + } + return { x: sortedX, y: sortedY }; +} + /** * Downsamples x/y arrays to at most maxPoints by taking every Nth point. * Always includes the first and last points to preserve the range. @@ -2810,11 +2839,10 @@ export class PlotlyChartProvider implements IChartProvider { yValues = binned.y; } if (xValues.length > 0) { - // Sort by x-value so lines don't zigzag - const indices = xValues.map((_, i) => i); - indices.sort((a, b) => (xValues[a]! < xValues[b]! ? -1 : xValues[a]! > xValues[b]! ? 1 : 0)); - let sortedX: unknown[] = indices.map(i => xValues[i]!); - let sortedY: number[] = indices.map(i => yValues[i]!); + // Sort by x-value so lines don't zigzag (no-op when already sorted). + const sorted = sortPointsByX(xValues, yValues); + let sortedX: unknown[] = sorted.x; + let sortedY: number[] = sorted.y; if (options.accumulate) { sortedY = accumulateValues(sortedY); } @@ -2841,10 +2869,8 @@ export class PlotlyChartProvider implements IChartProvider { if (shouldBin) { result = binAndAggregate(result.x, result.y, binSize, aggregation, xIsDateTime); } - // Sort by x-value so lines don't zigzag - const indices = result.x.map((_, i) => i); - indices.sort((a, b) => (result!.x[a]! < result!.x[b]! ? -1 : result!.x[a]! > result!.x[b]! ? 1 : 0)); - result = { x: indices.map(i => result!.x[i]!), y: indices.map(i => result!.y[i]!) }; + // Sort by x-value so lines don't zigzag (no-op when already sorted). + result = sortPointsByX(result.x, result.y); if (options.accumulate) { result = { x: result.x, y: accumulateValues(result.y) }; } diff --git a/src/Client/tests/unit/plotlyChartProvider.test.ts b/src/Client/tests/unit/plotlyChartProvider.test.ts index 8217966..40dab0e 100644 --- a/src/Client/tests/unit/plotlyChartProvider.test.ts +++ b/src/Client/tests/unit/plotlyChartProvider.test.ts @@ -1215,6 +1215,58 @@ describe('CompositeChartProvider', () => { expect(west.x).toEqual(['2024-01-01', '2024-01-02']); expect(west.y).toEqual([20, 25]); }); + + it('preserves order when data is already sorted by x (monotonic fast path)', () => { + // Already-sorted input exercises the no-allocation fast path. + // Output must still match input order exactly. + const table = makeTable( + [ + { name: 'timestamp', type: 'datetime' }, + { name: 'region', type: 'string' }, + { name: 'count', type: 'int' }, + ], + [ + ['2024-01-01', 'East', 10], + ['2024-01-02', 'East', 15], + ['2024-01-03', 'East', 20], + ['2024-01-04', 'East', 25], + ], + ); + const html = renderAndGetHtml(table, { + type: 'Line', + xColumn: 'timestamp', + yColumns: ['count'], + seriesColumns: ['region'], + }); + const east = parseTraces(html!)[0] as Record; + expect(east.x).toEqual(['2024-01-01', '2024-01-02', '2024-01-03', '2024-01-04']); + expect(east.y).toEqual([10, 15, 20, 25]); + }); + + it('preserves equal-x runs as stable in already-sorted input', () => { + // Non-decreasing (with equal x's) is the fast-path predicate; verify ties + // are kept in input order. + const table = makeTable( + [ + { name: 'timestamp', type: 'datetime' }, + { name: 'count', type: 'int' }, + ], + [ + ['2024-01-01', 1], + ['2024-01-01', 2], + ['2024-01-02', 3], + ['2024-01-02', 4], + ], + ); + const html = renderAndGetHtml(table, { + type: 'Line', + xColumn: 'timestamp', + yColumns: ['count'], + }); + const trace = parseTraces(html!)[0] as Record; + expect(trace.x).toEqual(['2024-01-01', '2024-01-01', '2024-01-02', '2024-01-02']); + expect(trace.y).toEqual([1, 2, 3, 4]); + }); }); // ─── Auto-inference ───────────────────────────────────────────── From e5c2666318a08b8f6ab2cf453fc162afb459939e Mon Sep 17 00:00:00 2001 From: Matt Warren Date: Tue, 28 Apr 2026 14:43:50 -0700 Subject: [PATCH 4/8] Remove unnecessary stringify then parse pattern --- src/Client/features/plotlyChartProvider.ts | 37 ++++++++++++++-------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/src/Client/features/plotlyChartProvider.ts b/src/Client/features/plotlyChartProvider.ts index af5d0ed..9629f10 100644 --- a/src/Client/features/plotlyChartProvider.ts +++ b/src/Client/features/plotlyChartProvider.ts @@ -1978,13 +1978,16 @@ export class PlotlyChartProvider implements IChartProvider { if (!root || !root.data) return undefined; const dataJson = JSON.stringify(root.data); - let layoutJson = root.layout ? JSON.stringify(root.layout) : '{}'; - if (darkMode) { - layoutJson = applyDarkModeToLayout(layoutJson); - } else { - layoutJson = applyLightModeToLayout(layoutJson); - } + // Apply theme to the in-memory layout object before the single stringify, + // so we don't pay an extra parse/stringify round-trip. + const layoutObj = (root.layout && typeof root.layout === 'object') + ? root.layout as Record + : {}; + const themedLayout = darkMode + ? applyDarkModeToLayout(layoutObj) + : applyLightModeToLayout(layoutObj); + const layoutJson = JSON.stringify(themedLayout); const configJson = root.config ? JSON.stringify(root.config) @@ -3061,9 +3064,12 @@ function applyCommonOptions(builder: PlotlyChartBuilder, options: ChartOptions): // ─── Dark Mode Layout Helper ──────────────────────────────────────────────── -function applyDarkModeToLayout(layoutJson: string): string { - const layout = (JSON.parse(layoutJson) as Record) ?? {}; - +/** + * Mutates `layout` in place with dark-mode background, font, and axis colors, + * and returns it. Operates on the in-memory object so the caller can avoid a + * parse/stringify round-trip. + */ +function applyDarkModeToLayout(layout: Record): Record { layout.paper_bgcolor = '#1e1e1e'; layout.plot_bgcolor = '#1e1e1e'; @@ -3088,7 +3094,7 @@ function applyDarkModeToLayout(layoutJson: string): string { applyDarkModeToAxis(scene, 'zaxis'); } - return JSON.stringify(layout); + return layout; } function applyDarkModeToAxis(parent: Record, axisKey: string): void { @@ -3102,9 +3108,12 @@ function applyDarkModeToAxis(parent: Record, axisKey: string): // ─── Light Mode Layout Helper ─────────────────────────────────────────────── -function applyLightModeToLayout(layoutJson: string): string { - const layout = (JSON.parse(layoutJson) as Record) ?? {}; - +/** + * Mutates `layout` in place with light-mode background, font, and axis colors, + * and returns it. Operates on the in-memory object so the caller can avoid a + * parse/stringify round-trip. + */ +function applyLightModeToLayout(layout: Record): Record { layout.paper_bgcolor = '#ffffff'; layout.plot_bgcolor = '#ffffff'; @@ -3129,7 +3138,7 @@ function applyLightModeToLayout(layoutJson: string): string { applyLightModeToAxis(scene, 'zaxis'); } - return JSON.stringify(layout); + return layout; } function applyLightModeToAxis(parent: Record, axisKey: string): void { From 871984d68ea974195beb64d211cad86414f54b30 Mon Sep 17 00:00:00 2001 From: Matt Warren Date: Tue, 28 Apr 2026 15:17:19 -0700 Subject: [PATCH 5/8] Move data to chart via message post instead of embedding literals in html. --- src/Client/features/plotlyChartProvider.ts | 95 +++++++++++++++++-- .../tests/unit/plotlyChartProvider.test.ts | 45 ++++++--- 2 files changed, 119 insertions(+), 21 deletions(-) diff --git a/src/Client/features/plotlyChartProvider.ts b/src/Client/features/plotlyChartProvider.ts index 9629f10..9dac9cd 100644 --- a/src/Client/features/plotlyChartProvider.ts +++ b/src/Client/features/plotlyChartProvider.ts @@ -554,6 +554,25 @@ class PlotlyChartBuilder { return createChartDiv(dataJson, layoutJson, configJson, divId); } + /** + * Returns the chart's data/layout/config as JSON strings ready to be sent + * to the webview page via a structured postMessage. The page handler does + * `JSON.parse` + `Plotly.newPlot` directly, skipping the round-trip through + * ``; +/** + * Result returned by the chart-rendering pipeline. + * + * - `html`: a pre-baked HTML body to drop into the chart region. Used for the + * raw-Plotly path (data already comes from the user's query as JSON) and for + * the multi-chart panel grid (multiple charts concatenated into one HTML + * string with positioning/styling). + * - `plot`: structured data for a single chart. The page handler builds the + * chart div and calls `Plotly.newPlot` directly with `JSON.parse`d inputs, + * avoiding `innerHTML` of a multi-MB HTML blob and the script-reinjection + * dance that comes with it. + */ +type ChartRenderResult = + | { type: 'html'; html: string } + | { type: 'plot'; divId: string; dataJson: string; layoutJson: string; configJson: string }; + /** View for Plotly charts rendered inside a webview. */ class PlotlyChartView implements IChartView { onCopyResult: ((pngDataUrl: string, svgDataUrl?: string) => void) | undefined; @@ -1730,7 +1793,7 @@ class PlotlyChartView implements IChartView { constructor( private readonly webview: IWebView, - private readonly render: (data: ResultTable, options: ChartOptions, darkMode: boolean) => string | undefined + private readonly render: (data: ResultTable, options: ChartOptions, darkMode: boolean) => ChartRenderResult | undefined ) { this.subscription = webview.handle((message) => { if (message.command === 'copyChartResult') { @@ -1751,15 +1814,24 @@ class PlotlyChartView implements IChartView { } renderChart(data: ResultTable, options: ChartOptions, darkMode: boolean): void { - const bodyHtml = this.render(data, options, darkMode); - if (bodyHtml) { - this.webview.setContent(bodyHtml); - } else { + const result = this.render(data, options, darkMode); + if (!result) { const typeName = escapeHtml(options.type || 'Unknown'); this.webview.setContent( `
` + `  Chart type "${typeName}" is not currently supported.
` ); + return; + } + if (result.type === 'html') { + this.webview.setContent(result.html); + } else { + this.webview.invoke('setChartContent', { + divId: result.divId, + dataJson: result.dataJson, + layoutJson: result.layoutJson, + configJson: result.configJson, + }); } } @@ -1847,28 +1919,31 @@ export class PlotlyChartProvider implements IChartProvider { return new PlotlyChartView(webview, (data, options, darkMode) => this.renderChartToHtmlDiv(data, options, darkMode)); } - private renderChartToHtmlDiv(data: ResultTable, options: ChartOptions, darkMode = false): string | undefined { + private renderChartToHtmlDiv(data: ResultTable, options: ChartOptions, darkMode = false): ChartRenderResult | undefined { // Allow the chart's mode option to override the editor theme if (options.mode === ChartMode.Light) darkMode = false; else if (options.mode === ChartMode.Dark) darkMode = true; if (options.type === ChartType.Plotly) { - return this.renderRawPlotlyChart(data, darkMode); + const html = this.renderRawPlotlyChart(data, darkMode); + return html ? { type: 'html', html } : undefined; } - // Multiple independent charts (CSS-scaled) + // Multiple independent charts (CSS-scaled). Concatenated into a single HTML + // string so the grid layout/scaling sits next to each chart. if (options.yLayout === ChartYLayout.SeparateCharts && (this.is2dChartType(options.type) || options.type === ChartType.Pie)) { const xColumn = this.get2dXColumn(data, options); if (xColumn) { const yColumns = this.get2dYColumns(data, options, xColumn); if (yColumns.length > 1) { - return this.renderMultiChartPanels(data, options, darkMode, yColumns); + const html = this.renderMultiChartPanels(data, options, darkMode, yColumns); + return html ? { type: 'html', html } : undefined; } } } const builder = this.buildChart(data, options, darkMode); - return builder?.toHtmlDiv(); + return builder?.toContent(); } /** Returns true for chart types that use 2D x/y column rendering and support y-split. */ diff --git a/src/Client/tests/unit/plotlyChartProvider.test.ts b/src/Client/tests/unit/plotlyChartProvider.test.ts index 40dab0e..d2339ec 100644 --- a/src/Client/tests/unit/plotlyChartProvider.test.ts +++ b/src/Client/tests/unit/plotlyChartProvider.test.ts @@ -199,12 +199,20 @@ describe('CompositeChartProvider', () => { }); describe('renderChart', () => { - it('calls webview.setContent() with chart HTML', () => { + it('posts chart content via webview.invoke() with parsed JSON payloads', () => { view.renderChart(make2dTable(), { type: 'Column' }, false); - expect(webview.setContent).toHaveBeenCalledOnce(); - const html = webview.setContent.mock.calls[0]![0] as string; - expect(html).toContain('plotly-chart'); - expect(html).toContain('Plotly.newPlot'); + expect(webview.invoke).toHaveBeenCalledOnce(); + const [command, args] = webview.invoke.mock.calls[0]!; + expect(command).toBe('setChartContent'); + const payload = args as { divId: string; dataJson: string; layoutJson: string; configJson: string }; + expect(payload.divId).toBe('plotly-chart'); + // dataJson must round-trip via JSON.parse to a non-empty trace array. + const traces = JSON.parse(payload.dataJson) as unknown[]; + expect(Array.isArray(traces)).toBe(true); + expect(traces.length).toBeGreaterThan(0); + // layoutJson and configJson must also be valid JSON. + expect(() => JSON.parse(payload.layoutJson)).not.toThrow(); + expect(() => JSON.parse(payload.configJson)).not.toThrow(); }); it('renders empty traces for table with no rows', () => { @@ -213,11 +221,11 @@ describe('CompositeChartProvider', () => { [], ); view.renderChart(emptyTable, { type: 'Column' }, false); - expect(webview.setContent).toHaveBeenCalledOnce(); - const html = webview.setContent.mock.calls[0]![0] as string; - const traces = html.match(/var data = JSON\.parse\('([\s\S]*?)'\);\s*var layout/); - expect(traces).toBeTruthy(); - const parsed = JSON.parse(decodeJsStringLiteral(traces![1]!)) as { x: unknown[]; y: unknown[] }[]; + expect(webview.invoke).toHaveBeenCalledOnce(); + const [command, args] = webview.invoke.mock.calls[0]!; + expect(command).toBe('setChartContent'); + const payload = args as { dataJson: string }; + const parsed = JSON.parse(payload.dataJson) as { x: unknown[]; y: unknown[] }[]; expect(parsed[0]!.x).toEqual([]); expect(parsed[0]!.y).toEqual([]); }); @@ -242,9 +250,24 @@ describe('CompositeChartProvider', () => { view = provider.createView(webview); }); - /** Helper to render and return the HTML sent to setContent. */ + /** + * Renders the chart and returns an HTML-shaped string suitable for the + * `parseTraces` / `parseLayout` regex helpers. For the normal single-chart + * path (which now goes through `webview.invoke('setChartContent', ...)`), + * we synthesize HTML in the same shape `createChartDiv` produces so the + * existing regex helpers continue to work unchanged. For the raw-Plotly + * and multi-chart panel paths (which still go through `setContent`), + * we return the HTML directly. + */ function renderAndGetHtml(table: ResultTable, options: ChartOptions, darkMode = false): string | undefined { view.renderChart(table, options, darkMode); + // Structured invoke: synthesize HTML using the same JSON.parse('...') format. + const invokeCall = webview.invoke.mock.calls.find((c: unknown[]) => c[0] === 'setChartContent'); + if (invokeCall) { + const p = invokeCall[1] as { divId: string; dataJson: string; layoutJson: string; configJson: string }; + const escape = (j: string) => j.replace(/\\/g, '\\\\').replace(/'/g, "\\'").replace(/<\//g, '<\\/'); + return `
\n`; + } if (webview.setContent.mock.calls.length === 0) return undefined; return webview.setContent.mock.calls[0]![0] as string; } From a496cb588d3b55b1f97349c71b820d3804ff9533 Mon Sep 17 00:00:00 2001 From: Matt Warren Date: Tue, 28 Apr 2026 16:41:57 -0700 Subject: [PATCH 6/8] Remove double redraw when in fill mode --- src/Client/features/plotlyChartProvider.ts | 25 ++++++++++++++++------ src/Client/features/resultsViewer.ts | 5 ++++- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/Client/features/plotlyChartProvider.ts b/src/Client/features/plotlyChartProvider.ts index 9dac9cd..5e926b7 100644 --- a/src/Client/features/plotlyChartProvider.ts +++ b/src/Client/features/plotlyChartProvider.ts @@ -1499,15 +1499,23 @@ const plotlyChartScripts = ` } if (!arValue) { - wrapperDiv.style.position = ''; - wrapperDiv.style.left = ''; - wrapperDiv.style.top = ''; + // Use absolute positioning so the wrapper is taken out of #chart's + // flow. If the wrapper participates in flow, even sub-pixel content- + // size changes from Plotly's internal layout can shift #chart's own + // clientWidth/Height, which retriggers our ResizeObserver and causes + // a second redundant resize. Aspect-ratio mode already works this way. + wrapperDiv.style.position = 'absolute'; + wrapperDiv.style.left = '0'; + wrapperDiv.style.top = '0'; wrapperDiv.style.width = availW + 'px'; wrapperDiv.style.height = availH + 'px'; wrapperDiv.style.visibility = 'visible'; lastAppliedW = chartDiv.clientWidth; lastAppliedH = chartDiv.clientHeight; - Plotly.newPlot(plotDiv, plotDiv.data, Object.assign({}, plotDiv.layout, buildLayoutOverrides(availW, availH)), plotDiv._context); + // Use relayout (not newPlot) so we don't re-upload trace data to the GPU + // for every resize. The width/height/font overrides are layout-only and + // relayout handles them efficiently. + Plotly.relayout(plotDiv, buildLayoutOverrides(availW, availH)); return; } var parts = arValue.split('/').map(Number); @@ -1533,7 +1541,8 @@ const plotlyChartScripts = ` wrapperDiv.style.visibility = 'visible'; lastAppliedW = chartDiv.clientWidth; lastAppliedH = chartDiv.clientHeight; - Plotly.newPlot(plotDiv, plotDiv.data, Object.assign({}, plotDiv.layout, buildLayoutOverrides(w, h)), plotDiv._context); + // Use relayout (not newPlot) so we don't re-upload trace data to the GPU. + Plotly.relayout(plotDiv, buildLayoutOverrides(w, h)); } // Expose for host code (tab switching, edit panel toggle, etc.) @@ -1610,9 +1619,11 @@ const plotlyChartScripts = ` wrapperDiv.style.visibility = 'visible'; } - // Apply font/size overrides to the already-rendered plot + // 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); lastAppliedW = availW; lastAppliedH = availH; diff --git a/src/Client/features/resultsViewer.ts b/src/Client/features/resultsViewer.ts index 0a72426..a44c749 100644 --- a/src/Client/features/resultsViewer.ts +++ b/src/Client/features/resultsViewer.ts @@ -1805,7 +1805,10 @@ class DocumentViewProvider implements vscode.CustomTextEditorProvider { } .view-content { display: none; height: 100%; overflow: hidden; } .view-content.active { display: flex; flex-direction: column; } - #chart.has-aspect-ratio.active { + #chart.active { + /* Always provide a containing block so the absolutely-positioned + * wrapper inside can't escape and so layout changes inside the + * wrapper don't bubble up to #chart and trigger ResizeObserver loops. */ position: relative; } #chart.has-aspect-ratio > :first-child { From b16db9b6e28561a56da41e3a6f990388eca9002a Mon Sep 17 00:00:00 2001 From: Matt Warren Date: Tue, 28 Apr 2026 16:47:20 -0700 Subject: [PATCH 7/8] Use requestAnimationFrame instead of timeout to know when draw after tab switch or resize --- src/Client/features/resultsViewer.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/Client/features/resultsViewer.ts b/src/Client/features/resultsViewer.ts index a44c749..e3f4ffd 100644 --- a/src/Client/features/resultsViewer.ts +++ b/src/Client/features/resultsViewer.ts @@ -1882,11 +1882,13 @@ class DocumentViewProvider implements vscode.CustomTextEditorProvider { editPanel.classList.add('visible'); } } - // Trigger chart resize when switching to chart + // Trigger chart resize when switching to chart. Use rAF (one frame, + // ~16ms) instead of a fixed 50ms timeout so the resize happens as + // soon as the browser has applied the new layout. if (viewId === 'chart') { - setTimeout(function() { + requestAnimationFrame(function() { if (window._chartResize) window._chartResize(); - }, 50); + }); } } @@ -1901,10 +1903,12 @@ class DocumentViewProvider implements vscode.CustomTextEditorProvider { window._vscodeApi.postMessage({ command: 'editPanelToggled', visible: editPanelUserVisible }); } - // Resize chart when panel toggles - setTimeout(function() { + // Resize chart when panel toggles. Use rAF (one frame, ~16ms) + // instead of a fixed 50ms timeout so the resize happens as soon as + // the browser has applied the panel's display change. + requestAnimationFrame(function() { if (window._chartResize) window._chartResize(); - }, 50); + }); } // Listen for messages from the extension From 0835da167e1ab6597b7b162c3ed5f162117696ae Mon Sep 17 00:00:00 2001 From: Matt Warren Date: Tue, 28 Apr 2026 17:16:51 -0700 Subject: [PATCH 8/8] Fixes from PR feedback Co-authored-by: Copilot --- src/Client/features/plotlyChartProvider.ts | 101 +++++++++++++++--- .../tests/unit/plotlyChartProvider.test.ts | 71 +++++++++++- 2 files changed, 152 insertions(+), 20 deletions(-) diff --git a/src/Client/features/plotlyChartProvider.ts b/src/Client/features/plotlyChartProvider.ts index 5e926b7..175b229 100644 --- a/src/Client/features/plotlyChartProvider.ts +++ b/src/Client/features/plotlyChartProvider.ts @@ -997,7 +997,11 @@ const PlotlyJsCdn = 'https://cdn.plot.ly/plotly-2.27.0.min.js'; * - `\\` (must come first) so JSON's own escapes survive JS-string decoding, * - `'` so we don't terminate the surrounding JS string, * - `` in the data can't break out of the - * enclosing ``; } if (webview.setContent.mock.calls.length === 0) return undefined; @@ -1058,6 +1096,31 @@ describe('CompositeChartProvider', () => { const html = renderAndGetHtml(table, { type: 'Plotly' }); expect(html).toContain('not currently supported'); }); + + it('escapes U+2028 / U+2029 line separators in embedded JSON', () => { + // U+2028 (line separator) and U+2029 (paragraph separator) were + // illegal inside JS string literals before ES2019 and remain a + // syntax-error risk on older runtimes. Verify they are escaped + // out of the embedded `JSON.parse('...')` literal. + const ls = '\u2028'; + const ps = '\u2029'; + const plotlyJson = JSON.stringify({ + data: [{ type: 'bar', x: ['ok' + ls + 'next'], y: [1] }], + layout: { title: 'pre' + ps + 'post' }, + }); + const table = makeTable( + [{ name: 'plotly_json', type: 'string' }], + [[plotlyJson]], + ); + const html = renderAndGetHtml(table, { type: 'Plotly' }); + expect(html).toBeDefined(); + // The literal characters must not appear in the produced script; + // they must be escape sequences (\u2028 / \u2029) instead. + expect(html).not.toContain(ls); + expect(html).not.toContain(ps); + expect(html).toContain('\\u2028'); + expect(html).toContain('\\u2029'); + }); }); // ─── Series columns ────────────────────────────────────────────