From ee4b3cd9e716c098d41a2d475572265eadf41f40 Mon Sep 17 00:00:00 2001 From: Monty Lennie Date: Mon, 13 Jul 2026 10:26:38 -0600 Subject: [PATCH] fix(sdk): eliminate Malloy chart flicker on re-render Charts rendered through the SDK's RenderedResult bridge flickered whenever the result re-rendered (a refetch that changed the data, a theme toggle, or a remount). The layout effect cleared the live container synchronously and only repainted after an async dynamic import resolved, so the browser painted one or more blank frames. Fill-sized charts made it worse because @malloydata/render paints them asynchronously after a resize pass. Double-buffer the swap instead. The live viz is held across renders; each new render paints into an offscreen-but-laid-out stage node and is swapped in only once the renderer signals onReady, disposing the old viz at that point. The old chart stays on screen until the new one has painted, so there is no blank frame on any trigger. A bounded fallback forces the swap if onReady never fires, so a failed render surfaces instead of leaving a stale chart on screen. Per-chart theme CSS variables are written on the stage rather than the shared container so the outgoing chart's chrome does not repaint mid-swap. The renderer chunk is pre-warmed so the first paint does not wait on the import. Also stop re-executing the chart-result queries on tab refocus and reconnect: a Malloy result is a pure function of the query text and filters (already in the query key), so a focus refetch only repaints the same result. The staleTime + refetchOnWindowFocus/onReconnect overrides are scoped to the result queries (QueryResult, ModelCell) rather than the global client, leaving other SDK queries on default freshness behavior. Signed-off-by: Monty Lennie --- .../sdk/src/components/Model/ModelCell.tsx | 2 + .../components/QueryResult/QueryResult.tsx | 2 + .../RenderedResult/RenderedResult.tsx | 204 +++++++++++++----- packages/sdk/src/utils/queryClient.ts | 14 ++ 4 files changed, 171 insertions(+), 51 deletions(-) diff --git a/packages/sdk/src/components/Model/ModelCell.tsx b/packages/sdk/src/components/Model/ModelCell.tsx index c6d2a0d61..95ff9221c 100644 --- a/packages/sdk/src/components/Model/ModelCell.tsx +++ b/packages/sdk/src/components/Model/ModelCell.tsx @@ -5,6 +5,7 @@ import React, { useEffect } from "react"; import { useQueryWithApiError } from "../../hooks/useQueryWithApiError"; import { usePublisherTheme } from "../../theme/ThemeContext"; import { parseResourceUri } from "../../utils/formatting"; +import { CHART_RESULT_QUERY_OPTIONS } from "../../utils/queryClient"; import { highlight } from "../highlighter"; import ResultContainer from "../RenderedResult/ResultContainer"; import ResultsDialog from "../ResultsDialog"; @@ -56,6 +57,7 @@ export function ModelCell({ }, ), enabled: runOnDemand ? hasRun : true, // Execute on demand or always + ...CHART_RESULT_QUERY_OPTIONS, }); const { mode } = usePublisherTheme(); diff --git a/packages/sdk/src/components/QueryResult/QueryResult.tsx b/packages/sdk/src/components/QueryResult/QueryResult.tsx index 307e8b713..0b257e59b 100644 --- a/packages/sdk/src/components/QueryResult/QueryResult.tsx +++ b/packages/sdk/src/components/QueryResult/QueryResult.tsx @@ -1,6 +1,7 @@ import { Suspense } from "react"; import { useQueryWithApiError } from "../../hooks/useQueryWithApiError"; import { parseResourceUri } from "../../utils/formatting"; +import { CHART_RESULT_QUERY_OPTIONS } from "../../utils/queryClient"; import { ApiErrorDisplay } from "../ApiErrorDisplay"; import { Loading } from "../Loading"; import ResultContainer from "../RenderedResult/ResultContainer"; @@ -90,6 +91,7 @@ export default function QueryResult({ versionId: versionId, }, ), + ...CHART_RESULT_QUERY_OPTIONS, }); return ( diff --git a/packages/sdk/src/components/RenderedResult/RenderedResult.tsx b/packages/sdk/src/components/RenderedResult/RenderedResult.tsx index 01337b533..741196b10 100644 --- a/packages/sdk/src/components/RenderedResult/RenderedResult.tsx +++ b/packages/sdk/src/components/RenderedResult/RenderedResult.tsx @@ -1,5 +1,5 @@ import { Box } from "@mui/material"; -import React, { Suspense, useLayoutEffect, useRef } from "react"; +import React, { Suspense, useEffect, useLayoutEffect, useRef } from "react"; import { buildMalloyExplicitTheme } from "../../theme/buildMalloyExplicitTheme"; import { buildTableCssVars } from "../../theme/buildTableCssVars"; import { buildVegaThemeOverride } from "../../theme/buildVegaThemeOverride"; @@ -20,6 +20,10 @@ interface MalloyVizHandle { setResult: (result: unknown) => void; render: (element: HTMLElement) => void; remove: () => void; + // Fires once the chart has painted (or immediately if it already has). We + // use it to swap a freshly-rendered chart in only when it's ready, so the + // previously-painted chart never blanks out mid-render. + onReady: (callback: () => void) => void; } declare global { @@ -67,6 +71,13 @@ const createRenderer = async ( return renderer.createViz() as MalloyVizHandle; }; +// Warm the renderer chunk as soon as this module loads so the first chart +// paint doesn't have to wait on the dynamic import resolving (the async +// import is what widened the clear-then-repaint gap into a visible flicker). +if (typeof window !== "undefined") { + void import("@malloydata/render"); +} + /** * Pull a per-chart Theme override out of a parsed Malloy result by reading * `# theme.*` annotations on the model and the query. Returns `undefined` @@ -243,39 +254,59 @@ function RenderedResultInner({ }: RenderedResultProps) { const ref = useRef(null); const hasMeasuredRef = useRef(false); + // The chart currently painted into the container, held across renders. A + // new render paints into a fresh offscreen stage and only swaps in (and + // disposes this one) once it's ready, so the container never blanks between + // charts. That up-front clear + cleanup dispose was the flicker. + const liveRef = useRef<{ viz: MalloyVizHandle; node: HTMLElement } | null>( + null, + ); + // Bumped on every render-effect run and on unmount. A slow async render that + // resolves after a newer one has started (or after unmount) checks this and + // bails, so overlapping renders can't leave two charts or leak a viz. + const renderGenRef = useRef(0); const { theme: baseTheme, layers, mode } = usePublisherTheme(); + // Dispose the last live viz on unmount only. Deliberately NOT done in the + // render effect's cleanup: a re-run must keep the old chart until the new + // one has painted, and the new render disposes it during the swap. + useEffect(() => { + return () => { + renderGenRef.current += 1; + liveRef.current?.viz.remove(); + liveRef.current = null; + }; + }, []); + useLayoutEffect(() => { if (!ref.current || !result) return; injectRendererOverrides(); - let isMounted = true; - // Track the active viz instance so the cleanup can dispose it even - // when unmount races with the async setup (see bug #3 from the code - // review: viz constructed but never `remove()`'d on rapid navigation). - let viz: MalloyVizHandle | undefined; const element = ref.current; - - while (element.firstChild) { - element.removeChild(element.firstChild); - } - - // Apply the shell theme's CSS variables immediately so the table - // chrome looks right during the async setup window. We re-apply once - // the per-chart override resolves below, so per-chart annotations - // affect both Vega and table styles. - applyTableCssVars(element, baseTheme); - - hasMeasuredRef.current = false; - + const myGen = (renderGenRef.current += 1); + let cancelled = false; + const isCurrent = () => !cancelled && myGen === renderGenRef.current; + // Created inside the async body; hoisted so cleanup can tear them down + // if this render is superseded before it swaps itself into `liveRef`. + let stage: HTMLDivElement | undefined; + let viz: MalloyVizHandle | undefined; let observer: MutationObserver | null = null; let measureTimeout: NodeJS.Timeout | null = null; + // Safety net so a render that never signals ready (an async renderer + // error that only reaches onError) can't leave a previous chart showing + // stale data forever; see the setTimeout below. + let readyFallback: ReturnType | null = null; + + hasMeasuredRef.current = false; - const measureRenderedSize = () => { - if (hasMeasuredRef.current || !isMounted || !element.firstElementChild) + // Measure the rendered chart's natural height off `root` (the stage that + // wraps the renderer output) and report it up. Same grandchild/dashboard + // HACK as before, just anchored on the stage wrapper. + const measureRenderedSize = (root: HTMLElement) => { + if (hasMeasuredRef.current || cancelled || !root.firstElementChild) return; - const child = element.firstElementChild as HTMLElement; + const child = root.firstElementChild as HTMLElement; const grandchild = child.firstElementChild as HTMLElement; if (!grandchild) return; const greatgrandchild = grandchild.firstElementChild as HTMLElement; @@ -315,52 +346,123 @@ function RenderedResultInner({ ? resolveTheme([...layers, perChart], mode) : baseTheme; - // Per-chart annotation may have shifted table CSS vars; re-apply. - if (isMounted) applyTableCssVars(element, effectiveTheme); + if (!isCurrent()) return; try { - const created = await createRenderer(effectiveTheme, onDrill); - if (!isMounted) { - // Unmounted during the dynamic import / construction. Tear - // down the viz we just made; the effect's cleanup won't see - // it because `viz` was unset. - created.remove(); - return; + viz = await createRenderer(effectiveTheme, onDrill); + } catch (error) { + console.error("Failed to create renderer:", error); + return; + } + if (!isCurrent()) { + // Superseded during the dynamic import / construction. + viz.remove(); + viz = undefined; + return; + } + + // Render into a fresh stage appended to the container. While a + // previous chart is still on screen, keep the stage laid out (so the + // fill chart actually measures a size and `onReady` fires) but hidden + // and overlaid; reveal it and drop the old chart only once painted. + const previous = liveRef.current; + stage = document.createElement("div"); + stage.style.width = "100%"; + stage.style.height = "100%"; + // Theme the chart's table/dashboard chrome on the stage itself, not on + // the shared container: during a swap the outgoing chart is still a + // child of the container, so writing the new per-chart CSS vars there + // would repaint the old chart's chrome to the new theme before it is + // swapped out. Scoping the vars to this stage keeps each chart stable. + applyTableCssVars(stage, effectiveTheme); + if (previous) { + element.style.position = "relative"; + stage.style.position = "absolute"; + stage.style.inset = "0"; + stage.style.visibility = "hidden"; + } + element.appendChild(stage); + + // Fallback measurement if `onReady` never fires: settle on DOM + // mutations, then measure once. + const stageNode = stage; + observer = new MutationObserver(() => { + if (measureTimeout) clearTimeout(measureTimeout); + measureTimeout = setTimeout(() => { + measureRenderedSize(stageNode); + observer?.disconnect(); + }, 100); + }); + observer.observe(stage, { + childList: true, + subtree: true, + attributes: true, + }); + + const activeViz = viz; + let promoted = false; + const promote = () => { + if (promoted || !isCurrent()) return; + promoted = true; + if (readyFallback) { + clearTimeout(readyFallback); + readyFallback = null; } - viz = created; - - observer = new MutationObserver(() => { - if (measureTimeout) clearTimeout(measureTimeout); - measureTimeout = setTimeout(() => { - measureRenderedSize(); - observer?.disconnect(); - }, 100); - }); - - observer.observe(element, { - childList: true, - subtree: true, - attributes: true, - }); + // The new chart has painted; drop the outgoing one now. + if (previous) { + previous.viz.remove(); + if (previous.node.parentNode === element) { + element.removeChild(previous.node); + } + } + stageNode.style.position = ""; + stageNode.style.inset = ""; + stageNode.style.visibility = ""; + element.style.position = ""; + liveRef.current = { viz: activeViz, node: stageNode }; + measureRenderedSize(stageNode); + }; + try { // The renderer accepts a Malloy Result; we don't import that type // in the SDK to avoid pinning to the malloy core types here. viz.setResult(parsed); - viz.render(element); + viz.render(stage); + viz.onReady(promote); + // If onReady never fires (an async render error that only reaches + // the renderer's onError), force the swap after a bounded wait so + // the outcome becomes visible instead of the previous chart + // lingering with stale data indefinitely. + readyFallback = setTimeout(promote, 10000); } catch (error) { console.error("Error rendering visualization:", error); observer?.disconnect(); - viz?.remove(); + viz.remove(); viz = undefined; + if (stageNode.parentNode === element) { + element.removeChild(stageNode); + } } })(); return () => { - isMounted = false; + cancelled = true; observer?.disconnect(); if (measureTimeout) clearTimeout(measureTimeout); - viz?.remove(); - viz = undefined; + if (readyFallback) clearTimeout(readyFallback); + // If this render built a stage but never swapped it into `liveRef` + // (superseded, or unmounted mid-render), tear it down so it can't + // leak a viz or leave an orphan node behind. + if (stage && liveRef.current?.node !== stage) { + viz?.remove(); + if (stage.parentNode === element) { + element.removeChild(stage); + } + // This render may have set the container position:relative for its + // overlay stage but never promoted; reset it so no stray relative + // lingers on the container. + element.style.position = ""; + } }; }, [result, onDrill, onSizeChange, baseTheme, layers, mode]); diff --git a/packages/sdk/src/utils/queryClient.ts b/packages/sdk/src/utils/queryClient.ts index 828d28c3c..db3f00501 100644 --- a/packages/sdk/src/utils/queryClient.ts +++ b/packages/sdk/src/utils/queryClient.ts @@ -13,3 +13,17 @@ export const globalQueryClient = new QueryClient({ }, }, }); + +// Refetch policy for chart/query-RESULT queries. A Malloy query result is a +// pure function of the query text + filters, which are already in each query +// key, so re-executing the query on tab refocus or reconnect only repaints the +// same result and can cause a visible chart flicker. Treat results as fresh +// for a few minutes and don't auto-refetch on focus/reconnect. Kept scoped to +// the result queries (spread into their useQuery options) rather than set on +// the global client, so other SDK queries (metadata lists, status) keep +// react-query's default freshness behavior. +export const CHART_RESULT_QUERY_OPTIONS = { + staleTime: 5 * 60 * 1000, + refetchOnWindowFocus: false, + refetchOnReconnect: false, +} as const;