From 43ea0509d6c6925ffa8cc56206ad209fcce9a509 Mon Sep 17 00:00:00 2001 From: chh-ay Date: Mon, 27 Jul 2026 00:07:32 +0700 Subject: [PATCH 1/2] fix(core): preserve cached worker view buffers --- .changeset/quiet-workers-paint.md | 5 + packages/core/src/worker-renderer.ts | 55 +++--- packages/core/test/worker-renderer.test.ts | 195 +++++++++++++++++++-- test/browser/vanilla-workbench.spec.ts | 58 ++++++ 4 files changed, 276 insertions(+), 37 deletions(-) create mode 100644 .changeset/quiet-workers-paint.md diff --git a/.changeset/quiet-workers-paint.md b/.changeset/quiet-workers-paint.md new file mode 100644 index 00000000..ac5d9f01 --- /dev/null +++ b/.changeset/quiet-workers-paint.md @@ -0,0 +1,5 @@ +--- +"@sheetwrite/core": patch +--- + +Keep cached worker-renderer views valid across repeated paints so scrolling no longer produces blank or stale regions. diff --git a/packages/core/src/worker-renderer.ts b/packages/core/src/worker-renderer.ts index 1978635f..125434ac 100644 --- a/packages/core/src/worker-renderer.ts +++ b/packages/core/src/worker-renderer.ts @@ -196,14 +196,24 @@ export class WorkerRenderer implements Renderer { return; } + // The renderer transfers only buffers it allocated: RenderCoordinator + // reuses view-owned buffers across repaints. + const ownedValueKinds = valueKinds.slice(); + const ownedNumberValues = numberValues.slice(); + const ownedStringPoolIds = stringPoolIds.slice(); + const ownedStringLocalIds = stringLocalIds.slice(); + const ownedStyleIds = view.styleIds.slice(); + const ownedStringPoolUpdateIds = stringPoolUpdateIds?.slice(); const transfers: Transferable[] = [ - valueKinds.buffer as ArrayBuffer, - numberValues.buffer as ArrayBuffer, - stringPoolIds.buffer as ArrayBuffer, - stringLocalIds.buffer as ArrayBuffer, - view.styleIds.buffer as ArrayBuffer, + ownedValueKinds.buffer as ArrayBuffer, + ownedNumberValues.buffer as ArrayBuffer, + ownedStringPoolIds.buffer as ArrayBuffer, + ownedStringLocalIds.buffer as ArrayBuffer, + ownedStyleIds.buffer as ArrayBuffer, ]; - if (stringPoolUpdateIds) transfers.push(stringPoolUpdateIds.buffer as ArrayBuffer); + if (ownedStringPoolUpdateIds) { + transfers.push(ownedStringPoolUpdateIds.buffer as ArrayBuffer); + } this.worker?.postMessage( { type: "paintPacked", @@ -211,12 +221,12 @@ export class WorkerRenderer implements Renderer { rows: view.rows, cols: view.cols, styles: view.styles, - styleIds: view.styleIds, - valueKinds, - numberValues, - stringPoolIds, - stringLocalIds, - stringPoolUpdateIds, + styleIds: ownedStyleIds, + valueKinds: ownedValueKinds, + numberValues: ownedNumberValues, + stringPoolIds: ownedStringPoolIds, + stringLocalIds: ownedStringLocalIds, + stringPoolUpdateIds: ownedStringPoolUpdateIds, stringPoolUpdateValues, localStrings, }, @@ -226,8 +236,9 @@ export class WorkerRenderer implements Renderer { } // Custom Store implementations may not expose raw arrays; keep the - // compatibility path, still transferring the fresh style-id buffer. - this.worker?.postMessage({ type: "paint", view }, [view.styleIds.buffer]); + // compatibility path while preserving ownership of their style IDs. + const styleIds = view.styleIds.slice(); + this.worker?.postMessage({ type: "paint", view: { ...view, styleIds } }, [styleIds.buffer]); } paintPanes(panes: readonly PanePaint[], divider: { x: number | null; y: number | null }): void { @@ -248,12 +259,12 @@ export class WorkerRenderer implements Renderer { rows: view.rows, cols: view.cols, styles: view.styles, - styleIds: view.styleIds, - valueKinds, - numberValues, - stringPoolIds, - stringLocalIds, - stringPoolUpdateIds, + styleIds: view.styleIds.slice(), + valueKinds: valueKinds.slice(), + numberValues: numberValues.slice(), + stringPoolIds: stringPoolIds.slice(), + stringLocalIds: stringLocalIds.slice(), + stringPoolUpdateIds: stringPoolUpdateIds?.slice(), stringPoolUpdateValues: view.stringPoolUpdateValues, localStrings: view.localStrings, } @@ -266,7 +277,9 @@ export class WorkerRenderer implements Renderer { packed.stringLocalIds.buffer as ArrayBuffer, packed.styleIds.buffer as ArrayBuffer, ); - if (stringPoolUpdateIds) transfers.push(stringPoolUpdateIds.buffer as ArrayBuffer); + if (packed.stringPoolUpdateIds) { + transfers.push(packed.stringPoolUpdateIds.buffer as ArrayBuffer); + } } return { diff --git a/packages/core/test/worker-renderer.test.ts b/packages/core/test/worker-renderer.test.ts index 665102b1..69fe682e 100644 --- a/packages/core/test/worker-renderer.test.ts +++ b/packages/core/test/worker-renderer.test.ts @@ -27,6 +27,16 @@ class RecordingWorker extends EventTarget { } } +class StructuredCloneWorker extends EventTarget { + readonly received: unknown[] = []; + + postMessage(message: unknown, transfer?: Transferable[]): void { + this.received.push(structuredClone(message, { transfer: transfer ?? [] })); + } + + terminate(): void {} +} + function latestWorker(): RecordingWorker { const worker = latestConstructedWorker; if (!worker) throw new Error("worker was not constructed"); @@ -56,10 +66,66 @@ interface SharedPaintPost { }; } +interface TransferablePackedArrays { + valueKinds: Uint8Array; + numberValues: Float64Array; + stringPoolIds: Uint32Array; + stringLocalIds: Int32Array; + styleIds: Uint32Array; + stringPoolUpdateIds?: Uint32Array; +} + +interface GenericPaintPost { + type: "paint"; + view: { + sheet: unknown; + styleIds: Uint32Array; + }; +} + +interface PanePaintPost { + type: "paintPanes"; + panes: unknown[]; +} + function isRecord(value: unknown): value is Record { return value !== null && typeof value === "object"; } +function isTransferablePackedArrays(value: unknown): value is TransferablePackedArrays { + return ( + isRecord(value) && + value.valueKinds instanceof Uint8Array && + value.numberValues instanceof Float64Array && + value.stringPoolIds instanceof Uint32Array && + value.stringLocalIds instanceof Int32Array && + value.styleIds instanceof Uint32Array && + (value.stringPoolUpdateIds === undefined || value.stringPoolUpdateIds instanceof Uint32Array) + ); +} + +function isGenericPaintPost(value: unknown): value is GenericPaintPost { + return ( + isRecord(value) && + value.type === "paint" && + isRecord(value.view) && + value.view.styleIds instanceof Uint32Array + ); +} + +function isPanePaintPost(value: unknown): value is PanePaintPost { + return isRecord(value) && value.type === "paintPanes" && Array.isArray(value.panes); +} + +function firstPackedPane(value: unknown): TransferablePackedArrays { + if (!isPanePaintPost(value)) throw new Error("expected pane paint message"); + const first = value.panes[0]; + if (!isRecord(first) || !isTransferablePackedArrays(first.packed)) { + throw new Error("expected packed first pane"); + } + return first.packed; +} + function isSharedPaintPost(value: unknown): value is SharedPaintPost { if (!isRecord(value) || value.type !== "paintPackedShared" || !isRecord(value.shared)) { return false; @@ -91,7 +157,7 @@ function makePackedView(): VisibleWindowView { } describe("WorkerRenderer", () => { - it("transfers the visible-window style id buffer when painting a generic view", () => { + it("transfers a renderer-owned style id buffer when painting a generic view", () => { const renderer = new WorkerRenderer(); const worker = new RecordingWorker(); Reflect.set(renderer, "worker", worker); @@ -110,8 +176,12 @@ describe("WorkerRenderer", () => { expect(worker.messages).toHaveLength(1); const posted = worker.messages[0]!; - expect(posted.message).toEqual({ type: "paint", view }); - expect(posted.transfer).toEqual([styleIds.buffer]); + if (!isGenericPaintPost(posted.message)) throw new Error("expected generic paint message"); + const postedView = posted.message.view; + expect(posted.message).toMatchObject({ type: "paint", view: { sheet: "s1" } }); + expect(postedView.styleIds).toEqual(styleIds); + expect(postedView.styleIds).not.toBe(styleIds); + expect(posted.transfer).toEqual([postedView.styleIds.buffer]); }); it("keeps the packed transfer path by default", () => { @@ -124,15 +194,71 @@ describe("WorkerRenderer", () => { expect(worker.messages).toHaveLength(1); const posted = worker.messages[0]!; + if (!isTransferablePackedArrays(posted.message)) { + throw new Error("expected packed paint message"); + } + const payload = posted.message; expect(posted.message).toMatchObject({ type: "paintPacked", sheet: "s1" }); expect(posted.transfer).toEqual([ - view.valueKinds!.buffer, - view.numberValues!.buffer, - view.stringPoolIds!.buffer, - view.stringLocalIds!.buffer, - view.styleIds.buffer, - view.stringPoolUpdateIds!.buffer, + payload.valueKinds.buffer, + payload.numberValues.buffer, + payload.stringPoolIds.buffer, + payload.stringLocalIds.buffer, + payload.styleIds.buffer, + payload.stringPoolUpdateIds!.buffer, ]); + expect(payload.valueKinds).not.toBe(view.valueKinds); + expect(payload.numberValues).not.toBe(view.numberValues); + expect(payload.stringPoolIds).not.toBe(view.stringPoolIds); + expect(payload.stringLocalIds).not.toBe(view.stringLocalIds); + expect(payload.styleIds).not.toBe(view.styleIds); + expect(payload.stringPoolUpdateIds).not.toBe(view.stringPoolUpdateIds); + }); + + it("keeps cached packed view buffers intact across repeated paints", () => { + const renderer = new WorkerRenderer(); + const worker = new StructuredCloneWorker(); + Reflect.set(renderer, "worker", worker); + const view = makePackedView(); + + renderer.paint(view); + renderer.paint(view); + + expect(worker.received).toHaveLength(2); + const second = worker.received[1]; + if (!isTransferablePackedArrays(second)) throw new Error("expected packed paint message"); + expect(second.valueKinds).toHaveLength(2); + expect(second.numberValues).toHaveLength(2); + expect(second.stringPoolIds).toHaveLength(2); + expect(second.stringLocalIds).toHaveLength(2); + expect(second.styleIds).toHaveLength(2); + expect(view.valueKinds).toHaveLength(2); + expect(view.numberValues).toHaveLength(2); + expect(view.stringPoolIds).toHaveLength(2); + expect(view.stringLocalIds).toHaveLength(2); + expect(view.styleIds).toHaveLength(2); + }); + + it("keeps cached generic view buffers intact across repeated paints", () => { + const renderer = new WorkerRenderer(); + const worker = new StructuredCloneWorker(); + Reflect.set(renderer, "worker", worker); + const view: VisibleWindowView = { + sheet: "s1", + rows: { start: 0, end: 1 }, + cols: [0, 1], + values: ["alpha", 42], + styleIds: new Uint32Array([0, 1]), + styles: [{}, { bold: true }], + }; + + renderer.paint(view); + renderer.paint(view); + + const second = worker.received[1]; + if (!isGenericPaintPost(second)) throw new Error("expected generic paint message"); + expect(second.view.styleIds).toHaveLength(2); + expect(view.styleIds).toHaveLength(2); }); it("preserves resolved hyperlink and conditional styles through the Worker payload", () => { @@ -263,14 +389,51 @@ describe("WorkerRenderer", () => { }, ], }); - expect(worker.messages[0]!.transfer).toEqual([ - packed.valueKinds!.buffer, - packed.numberValues!.buffer, - packed.stringPoolIds!.buffer, - packed.stringLocalIds!.buffer, - packed.styleIds.buffer, - packed.stringPoolUpdateIds!.buffer, + const posted = worker.messages[0]!; + const postedPacked = firstPackedPane(posted.message); + expect(posted.transfer).toEqual([ + postedPacked.valueKinds.buffer, + postedPacked.numberValues.buffer, + postedPacked.stringPoolIds.buffer, + postedPacked.stringLocalIds.buffer, + postedPacked.styleIds.buffer, + postedPacked.stringPoolUpdateIds!.buffer, ]); + expect(postedPacked.valueKinds).not.toBe(packed.valueKinds); + expect(postedPacked.styleIds).not.toBe(packed.styleIds); + }); + + it("keeps cached packed pane buffers intact across repeated paints", () => { + const renderer = new WorkerRenderer(); + const worker = new StructuredCloneWorker(); + Reflect.set(renderer, "worker", worker); + const view = makePackedView(); + const panes: PanePaint[] = [ + { + view, + clip: { x: 0, y: 0, w: 100, h: 40 }, + scrollTop: 0, + scrollLeft: 0, + }, + ]; + + renderer.paintPanes(panes, { x: null, y: null }); + renderer.paintPanes(panes, { x: null, y: null }); + + expect(worker.received).toHaveLength(2); + const second = firstPackedPane(worker.received[1]); + expect(second.valueKinds).toHaveLength(2); + expect(second.numberValues).toHaveLength(2); + expect(second.stringPoolIds).toHaveLength(2); + expect(second.stringLocalIds).toHaveLength(2); + expect(second.styleIds).toHaveLength(2); + expect(second.stringPoolUpdateIds).toHaveLength(1); + expect(view.valueKinds).toHaveLength(2); + expect(view.numberValues).toHaveLength(2); + expect(view.stringPoolIds).toHaveLength(2); + expect(view.stringLocalIds).toHaveLength(2); + expect(view.styleIds).toHaveLength(2); + expect(view.stringPoolUpdateIds).toHaveLength(1); }); it("round-trips sender lifecycle payloads through the worker handler before acknowledging", () => { diff --git a/test/browser/vanilla-workbench.spec.ts b/test/browser/vanilla-workbench.spec.ts index 0970e408..2b86de9e 100644 --- a/test/browser/vanilla-workbench.spec.ts +++ b/test/browser/vanilla-workbench.spec.ts @@ -299,6 +299,64 @@ test("renderer selection is construction-bound and deep-linked", { expect(errors.console).toEqual([]); }); +test("worker repaint keeps a cached non-shared view painted after a sub-row scroll", async ({ + page, +}) => { + const errors = collectErrors(page); + await page.goto(`${VANILLA_URL}?renderer=worker`); + await waitForLive(page); + await expect(page.getByTestId("renderer")).toContainText( + "Requested: Web Worker ยท Active: Web Worker", + { timeout: 20_000 }, + ); + await expect(page.getByTestId("renderer")).toHaveAttribute("data-fallback-count", "0"); + await expect + .poll(async () => Number((await page.locator(CANVAS).getAttribute("data-worker-frame")) ?? 0), { + timeout: 20_000, + message: "Worker never acknowledged the initial frame", + }) + .toBeGreaterThan(0); + await expect.poll(() => canvasBodyPainted(page)).toBe(true); + expect(await page.evaluate(() => globalThis.crossOriginIsolated)).toBe(false); + + const scroller = page.locator(`${GRID} .sheetwrite-scroller`); + await scroller.hover(); + const initialFrame = Number((await page.locator(CANVAS).getAttribute("data-worker-frame")) ?? 0); + await page.mouse.wheel(0, 8); + await expect.poll(() => scroller.evaluate((element) => element.scrollTop)).toBeGreaterThan(0); + await expect + .poll(async () => Number((await page.locator(CANVAS).getAttribute("data-worker-frame")) ?? 0), { + timeout: 20_000, + message: "Worker did not paint the first sub-row scroll", + }) + .toBeGreaterThan(initialFrame); + + const frameBeforeCachedPaint = Number( + (await page.locator(CANVAS).getAttribute("data-worker-frame")) ?? 0, + ); + const cellsBeforeCachedPaint = await gridCellTexts(page); + const scrollBeforeCachedPaint = await scroller.evaluate((element) => element.scrollTop); + await page.mouse.wheel(0, 8); + await expect + .poll(() => scroller.evaluate((element) => element.scrollTop)) + .toBeGreaterThan(scrollBeforeCachedPaint); + const scrollAfterCachedPaint = await scroller.evaluate((element) => element.scrollTop); + expect(scrollAfterCachedPaint - scrollBeforeCachedPaint).toBeLessThan(20); + // The second fractional scroll stays inside the same row window, so the data + // signature and visible cells stay fixed while another Worker frame paints. + expect(await gridCellTexts(page)).toEqual(cellsBeforeCachedPaint); + await expect + .poll(async () => Number((await page.locator(CANVAS).getAttribute("data-worker-frame")) ?? 0), { + timeout: 20_000, + message: "Worker did not repaint the cached view after a sub-row scroll", + }) + .toBeGreaterThan(frameBeforeCachedPaint); + await expect.poll(() => canvasBodyPainted(page)).toBe(true); + expect(errors.page).toEqual([]); + expect(errors.worker).toEqual([]); + expect(errors.console).toEqual([]); +}); + test("a failed Worker boot falls back honestly to the main thread", { tag: "@portability", }, async ({ page }) => { From 4b4da591e15b4bc0d96566e4bbf9f2cac4e9821a Mon Sep 17 00:00:00 2001 From: chh-ay Date: Mon, 27 Jul 2026 00:09:17 +0700 Subject: [PATCH 2/2] test(browser): cover cached worker repaints --- test/browser/vanilla-workbench.spec.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/browser/vanilla-workbench.spec.ts b/test/browser/vanilla-workbench.spec.ts index 2b86de9e..aff184fd 100644 --- a/test/browser/vanilla-workbench.spec.ts +++ b/test/browser/vanilla-workbench.spec.ts @@ -299,9 +299,9 @@ test("renderer selection is construction-bound and deep-linked", { expect(errors.console).toEqual([]); }); -test("worker repaint keeps a cached non-shared view painted after a sub-row scroll", async ({ - page, -}) => { +test("worker repaint keeps a cached non-shared view painted after a sub-row scroll", { + tag: "@portability", +}, async ({ page }) => { const errors = collectErrors(page); await page.goto(`${VANILLA_URL}?renderer=worker`); await waitForLive(page); @@ -316,7 +316,7 @@ test("worker repaint keeps a cached non-shared view painted after a sub-row scro message: "Worker never acknowledged the initial frame", }) .toBeGreaterThan(0); - await expect.poll(() => canvasBodyPainted(page)).toBe(true); + await expect.poll(() => canvasBodyPainted(page), { timeout: 20_000 }).toBe(true); expect(await page.evaluate(() => globalThis.crossOriginIsolated)).toBe(false); const scroller = page.locator(`${GRID} .sheetwrite-scroller`); @@ -351,7 +351,7 @@ test("worker repaint keeps a cached non-shared view painted after a sub-row scro message: "Worker did not repaint the cached view after a sub-row scroll", }) .toBeGreaterThan(frameBeforeCachedPaint); - await expect.poll(() => canvasBodyPainted(page)).toBe(true); + await expect.poll(() => canvasBodyPainted(page), { timeout: 20_000 }).toBe(true); expect(errors.page).toEqual([]); expect(errors.worker).toEqual([]); expect(errors.console).toEqual([]);