diff --git a/CHANGELOG.md b/CHANGELOG.md index 96353e36..2806d614 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -156,6 +156,21 @@ auto-generated per-PR notes; this file is the curated, human-readable history. clicks are inert instead of a TypeError). ### Fixed +- **The detached Data view keeps its committed result visible during streaming + reruns** (#198). Changing a filter or clicking **Refresh** used to repaint the + in-flight result on every network chunk, which (a) flashed `Query returned 0 + rows.` whenever a chunk carried column metadata before any data rows, and (b) + destroyed and recreated the Panel's Chart.js instance on every chunk. The + detached view now follows the same commit-on-success policy as dashboard + tiles: streaming updates a lightweight `Running… rows read` status only, + the previously committed Table/JSON/Panel (and its chart) stays on screen + untouched, and the new result is committed and repainted exactly once after a + successful current-generation completion. Failure, cancellation, supersession, + and close all keep the previous committed result — and because the in-flight + result is never painted, the old failure-time restore repaint is gone. Winning + bound parameters are still recorded once; the committed toolbar row count no + longer changes mid-stream (`src/ui/results.js`). Dashboard and workbench + streaming behavior are unchanged. - **The detached view's Logs (Panel) surface now scrolls** (#185 follow-up). A readonly panel renders straight into the block-level `.res-body` with no `.panel-body` flex wrapper, so the `flex:1; min-height:0` that bounds diff --git a/src/ui/results.js b/src/ui/results.js index c9a2879d..c2802b03 100644 --- a/src/ui/results.js +++ b/src/ui/results.js @@ -675,9 +675,12 @@ export function expandDataPane(app, r) { let statusEl = null; const inner = h('div', { class: 'res-body' }); - // Render `res` (defaults to the committed `current`) into the body. A - // rerun paints the in-flight result progressively as it streams, then - // paints `current` once it commits (or reverts to `current` on failure). + // Render `res` (defaults to the committed `current`) into the body. + // Commit-on-success (#198): a rerun NEVER paints its in-flight result — + // paint runs only on view changes, local sorting, the initial render, and + // a successful current-generation commit. So the previous committed result + // stays on screen through streaming (no metadata-only "0 rows" flash) and + // the chart is not destroyed/recreated per chunk. const paint = (res = current) => withDocument(doc, () => { // Destroy the previous chart before rebuilding — same reasoning as // renderResults' destroy-before-rebuild (nothing may leak its canvas). @@ -739,7 +742,7 @@ export function expandDataPane(app, r) { if (blockers.length) { settle('Enter a value for: ' + blockers.join(', ')); return; } if (src.errors.length) { settle(src.errors[0]); return; } running = true; - setStatus(''); + setStatus('Running…'); if (refreshBtn) refreshBtn.disabled = true; if (!(await app.ensureFreshToken())) { if (myGen === gen && !closed) settle('Not signed in'); @@ -753,21 +756,25 @@ export function expandDataPane(app, r) { // Native param_ bindings + the captured session (when any). params: { ...(sessionId ? { session_id: sessionId } : {}), ...mergedSourceArgs(src) }, signal, - // Progressive streaming: paint the in-flight result as rows arrive. - onChunk: () => { if (myGen === gen && !closed) paint(result); }, + // Progress-only streaming (#198): update the lightweight status text as + // rows arrive, but NEVER paint the in-flight result and NEVER touch the + // committed `current` / stat / view / chart — only the winning + // completion below commits and repaints. A stale/closed chunk is dropped. + onChunk: () => { + if (myGen !== gen || closed) return; + const rowsRead = Number(result.progress?.rows) || 0; + setStatus(rowsRead > 0 ? `Running… ${formatRows(rowsRead)} rows read` : 'Running…'); + }, }); if (myGen !== gen || closed) return; // superseded or closed → discard silently - if (result.error) { - // A failed refresh keeps the previous result visible (revert any - // partial streamed rows) and reports the error in the status line. - settle(result.error); - paint(current); - return; - } - settle(''); + // The in-flight result was never painted, so failure/cancel needs no + // restore repaint — the committed `current` is still on screen (#198). + if (result.cancelled) { settle(''); return; } + if (result.error) { settle(result.error); return; } current = result; // #171: record the winning run's bound params via the shared recorder. app.recordBoundParams(src.statements.flatMap((s) => s.boundParams)); + settle(''); paint(); } diff --git a/tests/unit/results.test.js b/tests/unit/results.test.js index d6d8008c..4a10ce50 100644 --- a/tests/unit/results.test.js +++ b/tests/unit/results.test.js @@ -2,6 +2,7 @@ import { describe, it, expect, vi } from 'vitest'; import { renderResults, renderJson, renderTable, openCellDetail, openRowsViewer, expandDataPane } from '../../src/ui/results.js'; import { makeApp } from '../helpers/fake-app.js'; import { newResult } from '../../src/core/stream.js'; +import { formatRows } from '../../src/core/format.js'; const click = (el) => el.dispatchEvent(new Event('click', { bubbles: true })); // A genuine backdrop click: mousedown and click both land on `el` itself @@ -726,7 +727,7 @@ describe('expandDataPane', () => { runReadInto: vi.fn(async (result, opts) => { result.columns = [{ name: 'n', type: 'UInt64' }]; result.rows = [[opts.params.param_level]]; - opts.onChunk(); // simulate a streamed chunk → progressive repaint + opts.onChunk(); // a streamed chunk → progress-only status, no repaint (#198) return result; }), }); @@ -851,6 +852,147 @@ describe('expandDataPane', () => { expect(refreshBtn(overlay).disabled).toBe(false); expect(overlay.querySelector('.detached-status').textContent).toContain('Enter a value for: level'); }); + + // ── commit-on-success streaming policy (#198) ────────────────────────────── + // A controllable runReadInto: captures the in-flight result + onChunk so a + // test can emit chunks and resolve on its own schedule. `chunk(patch)` merges + // into the result then fires onChunk; `finish(patch)` merges then resolves. + const deferredRun = () => { + const ctl = { runs: [] }; + ctl.fn = vi.fn((result, opts) => new Promise((resolve) => { + const run = { + result, + chunk: (patch = {}) => { Object.assign(result, patch); opts.onChunk(); }, + finish: (patch = {}) => { Object.assign(result, patch); resolve(result); }, + }; + ctl.runs.push(run); + ctl.last = run; + })); + return ctl; + }; + // A chart-shaped result whose captured source declares a `{region:String}` + // param, so the detached view renders the filter row + Refresh AND its Panel + // view auto-resolves to a chart. + const chartParamResult = () => { + const r = chartResult(); + r.source = { ...r.source, sql: 'SELECT carrier, region, flights, delay FROM flights WHERE region = {region:String}', rowLimit: 100 }; + return r; + }; + + it('keeps the previous committed result visible during streaming and never flashes "Query returned 0 rows."', async () => { + const run = deferredRun(); + const app = makeApp({ runReadInto: run.fn }); + app.state.varValues.level = 'Warning'; + expandDataPane(app, paramResult()); + const overlay = document.querySelector('.graph-overlay'); + click(refreshBtn(overlay)); + await tick(); + expect(refreshBtn(overlay).disabled).toBe(true); + expect(overlay.querySelector('.detached-status').textContent).toBe('Running…'); + // metadata-only chunk: columns present, zero rows — must NOT flash "0 rows". + run.last.chunk({ columns: [{ name: 'n', type: 'UInt64' }], rows: [], progress: { rows: 0, bytes: 0, elapsed_ns: 0 } }); + expect(overlay.textContent).not.toContain('Query returned 0 rows.'); + expect(overlay.querySelectorAll('.res-table tbody tr')).toHaveLength(2); // previous result intact + expect(overlay.querySelector('.detached-status').textContent).toBe('Running…'); + // a data chunk carrying a progress counter → status reports rows read. + run.last.chunk({ rows: [['x']], progress: { rows: 12400, bytes: 0, elapsed_ns: 0 } }); + expect(overlay.querySelectorAll('.res-table tbody tr')).toHaveLength(2); // STILL the previous result + expect(overlay.querySelector('.stat .v').textContent).toBe('2 rows'); // committed count unchanged while streaming + expect(overlay.querySelector('.detached-status').textContent).toBe(`Running… ${formatRows(12400)} rows read`); + // resolve → commit exactly once. + run.last.finish(); + await tick(); + expect(overlay.querySelectorAll('.res-table tbody tr')).toHaveLength(1); // the new result is now committed + expect(overlay.querySelector('.detached-status').textContent).toBe(''); + expect(refreshBtn(overlay).disabled).toBe(false); + }); + + it('commits exactly once on success: before completion Copy/recents target the OLD result, after they target the NEW', async () => { + const run = deferredRun(); + const app = makeApp({ runReadInto: run.fn }); + app.state.varValues.level = 'Warning'; + expandDataPane(app, paramResult()); + const overlay = document.querySelector('.graph-overlay'); + click(refreshBtn(overlay)); + await tick(); + // in-flight rows exist, but nothing is committed yet. + run.last.chunk({ columns: [{ name: 'n', type: 'UInt64' }], rows: [['NEW']], progress: { rows: 1, bytes: 0, elapsed_ns: 0 } }); + expect(overlay.querySelector('.res-table tbody td.cell').textContent).toBe('2'); // still the old snapshot + expect(app.recordBoundParams).not.toHaveBeenCalled(); + click([...overlay.querySelectorAll('.res-act')].find((b) => b.textContent.includes('Copy'))); + expect(app.actions.copySnapshot.mock.calls.at(-1)[0].rows).toEqual([['2', 'b'], ['1', null]]); // Copy = OLD result + // resolve → commit. + run.last.finish(); + await tick(); + expect(overlay.querySelector('.res-table tbody td.cell').textContent).toBe('NEW'); + expect(app.recordBoundParams).toHaveBeenCalledTimes(1); // recents recorded exactly once, on success + click([...overlay.querySelectorAll('.res-act')].find((b) => b.textContent.includes('Copy'))); + expect(app.actions.copySnapshot.mock.calls.at(-1)[0].rows).toEqual([['NEW']]); // Copy = NEW result + }); + + it('Panel: streaming chunks do not churn the chart; a successful commit destroys the old chart once and creates one replacement', async () => { + const run = deferredRun(); + const app = makeApp({ runReadInto: run.fn }); + const instances = []; + const RealChart = app.Chart; + app.Chart = class extends RealChart { constructor(...a) { super(...a); instances.push(this); } }; + app.state.varValues.region = 'E'; + expandDataPane(app, chartParamResult()); + const overlay = document.querySelector('.graph-overlay'); + click([...overlay.querySelectorAll('.result-view-tab')].find((b) => b.textContent === 'Panel')); + expect(instances).toHaveLength(1); // the committed snapshot's chart + const chart0 = instances[0]; + expect(chart0.destroyed).toBe(false); + // Refresh → stream several chunks WITHOUT resolving. + click(refreshBtn(overlay)); + await tick(); + run.last.chunk({ progress: { rows: 100, bytes: 0, elapsed_ns: 0 } }); + run.last.chunk({ progress: { rows: 200, bytes: 0, elapsed_ns: 0 } }); + expect(chart0.destroyed).toBe(false); // not churned by chunks + expect(instances).toHaveLength(1); // no per-chunk chart rebuild + // resolve successfully → one destroy + one replacement. + run.last.finish({ columns: chartResult().columns, rows: [['B6', 'E', '30', '1.1']] }); + await tick(); + expect(chart0.destroyed).toBe(true); + expect(instances).toHaveLength(2); + }); + + it('a current-generation cancelled result never replaces the committed result and records nothing', async () => { + const run = deferredRun(); + const app = makeApp({ runReadInto: run.fn }); + app.state.varValues.level = 'Warning'; + expandDataPane(app, paramResult()); + const overlay = document.querySelector('.graph-overlay'); + click(refreshBtn(overlay)); + await tick(); + run.last.chunk({ columns: [{ name: 'n', type: 'UInt64' }], rows: [['NEW']], progress: { rows: 1, bytes: 0, elapsed_ns: 0 } }); + run.last.finish({ cancelled: true }); + await tick(); + expect(overlay.querySelectorAll('.res-table tbody tr')).toHaveLength(2); // previous result kept + expect(overlay.querySelector('.res-table tbody td.cell').textContent).toBe('2'); + expect(app.recordBoundParams).not.toHaveBeenCalled(); + expect(overlay.querySelector('.detached-status').textContent).toBe(''); // cancel clears the status + expect(refreshBtn(overlay).disabled).toBe(false); + }); + + it('a late chunk from a superseded run does not update the status; only the newest run controls it', async () => { + const run = deferredRun(); + const app = makeApp({ runReadInto: run.fn }); + app.state.varValues.level = 'A'; + expandDataPane(app, paramResult()); + const overlay = document.querySelector('.graph-overlay'); + click(refreshBtn(overlay)); // run 1 + await tick(); + const run1 = run.last; + app.state.varValues.level = 'B'; + click(refreshBtn(overlay)); // run 2 supersedes (aborts run 1) + await tick(); + const run2 = run.last; + run1.chunk({ progress: { rows: 99999 } }); // a late chunk from the superseded run + expect(overlay.querySelector('.detached-status').textContent).toBe('Running…'); // NOT run 1's count + run2.chunk({ progress: { rows: 5 } }); // the current run drives the status + expect(overlay.querySelector('.detached-status').textContent).toBe(`Running… ${formatRows(5)} rows read`); + }); }); describe('renderJson', () => {