From 81b6ae1f9d80268c753e0d5f800ae79e66df14b2 Mon Sep 17 00:00:00 2001 From: u8array Date: Sat, 16 May 2026 09:02:25 +0200 Subject: [PATCH 1/3] perf(preview): cache the Labelary blob URL by ZPL string Toggling preview off then on for a side-by-side pixel compare with the editor used to fire a fresh fetch every time, even when nothing changed. Keep the (zpl, url) pair in a module-level closure with get/set semantics; the closure owns the URL so callers can't forget to revoke the stale blob on a cache miss. The slot lives outside Zustand state because the blob URL is a non- serialisable side-effect handle: persisting it through `partialize` would resurrect a stale identifier across reloads, and putting it in the store would churn every selector that observes previewMode. Two test cases exercise the cache hit + cache miss + revoke paths; a test-only `__resetPreviewCacheForTests` keeps them isolated since the closure outlives `useLabelStore.setState`. --- src/store/labelStore.test.ts | 77 +++++++++++++++++++++++++++++++++++- src/store/labelStore.ts | 51 ++++++++++++++++++++++-- 2 files changed, 122 insertions(+), 6 deletions(-) diff --git a/src/store/labelStore.test.ts b/src/store/labelStore.test.ts index b86d11b2..10297c1f 100644 --- a/src/store/labelStore.test.ts +++ b/src/store/labelStore.test.ts @@ -1,8 +1,34 @@ -import { describe, it, expect, beforeEach } from 'vitest'; -import { useLabelStore, currentObjects } from './labelStore'; +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import type * as Labelary from '../lib/labelary'; +import { + useLabelStore, + currentObjects, + __resetPreviewCacheForTests, +} from './labelStore'; import { isGroup, type LabelObject } from '../types/Group'; import { defined, props } from '../test/helpers'; +// Mock the Labelary client so cache tests run without network I/O. +// `fetchPreview` returns a unique blob URL on each call so the test can +// see when a re-fetch happened vs. a cache hit (same URL = cached). +vi.mock('../lib/labelary', async (importOriginal) => { + const actual = await importOriginal(); + let counter = 0; + return { + ...actual, + fetchPreview: vi.fn(async () => `blob:mock-${++counter}`), + }; +}); + +// URL.revokeObjectURL is a no-op stub in jsdom by default but emits a +// warning in some setups; provide an explicit spy so we can also assert +// the cache revokes the previous URL on miss without flagging warnings. +// Also drop any cached preview entry so the cache tests stay isolated. +beforeEach(() => { + globalThis.URL.revokeObjectURL = vi.fn(); + __resetPreviewCacheForTests(); +}); + /** Reset store to clean state before each test. */ function reset() { useLabelStore.setState({ @@ -943,4 +969,51 @@ describe('ungroup', () => { expect(state().previewMode.status).toBe('idle'); }); }); + + describe('preview cache', () => { + function activeUrl(): string | null { + const mode = state().previewMode; + return mode.status === 'active' ? mode.url : null; + } + + it('skips the fetch when the ZPL is unchanged across toggles', async () => { + const labelary = await import('../lib/labelary'); + const fetchSpy = vi.mocked(labelary.fetchPreview); + fetchSpy.mockClear(); + + await state().enterPreviewMode(); + expect(state().previewMode.status).toBe('active'); + expect(fetchSpy).toHaveBeenCalledTimes(1); + const firstUrl = activeUrl(); + + state().exitPreviewMode(); + expect(state().previewMode.status).toBe('idle'); + + // Re-open with no changes: cache hit, no extra fetch, same URL. + await state().enterPreviewMode(); + expect(fetchSpy).toHaveBeenCalledTimes(1); + expect(activeUrl()).toBe(firstUrl); + }); + + it('re-fetches and revokes the stale URL when the ZPL changes', async () => { + const labelary = await import('../lib/labelary'); + const fetchSpy = vi.mocked(labelary.fetchPreview); + fetchSpy.mockClear(); + const revokeSpy = vi.mocked(globalThis.URL.revokeObjectURL); + + await state().enterPreviewMode(); + const firstUrl = activeUrl(); + expect(firstUrl).toBeTruthy(); + state().exitPreviewMode(); + + // Mutate the design — the next enterPreviewMode generates different + // ZPL and must hit the API again, revoking the prior blob. + state().addObject('text'); + await state().enterPreviewMode(); + + expect(fetchSpy).toHaveBeenCalledTimes(2); + expect(revokeSpy).toHaveBeenCalledWith(firstUrl); + expect(activeUrl()).not.toBe(firstUrl); + }); + }); }); diff --git a/src/store/labelStore.ts b/src/store/labelStore.ts index 2c239b7e..8e35846a 100644 --- a/src/store/labelStore.ts +++ b/src/store/labelStore.ts @@ -250,6 +250,42 @@ function updateCurrentObjects( * multiplies it by pasteCount because the clipboard source stays put. */ const DUPLICATE_OFFSET_DOTS = 20; +/** Single-entry cache for the Labelary preview blob URL, keyed by the + * exact ZPL string that produced it. Module-level rather than store- + * state because the blob URL is a non-serialisable side-effect handle: + * persisting it through `partialize` would resurrect a stale identifier + * across reloads, and including it in Zustand state would churn every + * selector that observes the store. + * + * The closure owns the URL: `set` revokes the previous blob before + * replacing it, so callers can't leak by forgetting to clean up. */ +const previewCache = (() => { + let entry: { zpl: string; url: string } | null = null; + return { + /** Returns the cached URL if `zpl` matches the cached key, else null. */ + get(zpl: string): string | null { + return entry && entry.zpl === zpl ? entry.url : null; + }, + /** Stores a fresh (zpl, url) pair. Revokes the previously held URL + * if any so the browser can reclaim the blob memory. */ + set(zpl: string, url: string): void { + if (entry) URL.revokeObjectURL(entry.url); + entry = { zpl, url }; + }, + /** Test-only: drop the cached entry without revoking, so a fresh + * test starts from a clean slate. Production callers should never + * need this — `set` handles eviction on its own. */ + _resetForTests(): void { + entry = null; + }, + }; +})(); + +/** Test-only handle to clear the preview cache between test cases. + * Marked underscored to discourage production use; the cache otherwise + * manages its own lifecycle via the `set` revoke path. */ +export const __resetPreviewCacheForTests = (): void => previewCache._resetForTests(); + /** Build offset copies of objects identified by `ids`. Missing ids are * silently dropped. Props are shallow-cloned to match the pattern in * copySelectedObjects — even though no current code path mutates props, @@ -840,9 +876,16 @@ export const useLabelStore = create()( if (state.previewMode.status === 'loading' || state.previewMode.status === 'active') { return; } - set({ previewMode: { status: 'loading' } }); const objs = currentObjects(state); const zpl = generateZPL(state.label, objs); + // Toggling preview off then on for a side-by-side pixel compare + // shouldn't burn an API call when nothing changed. + const cachedUrl = previewCache.get(zpl); + if (cachedUrl !== null) { + set({ previewMode: { status: 'active', url: cachedUrl } }); + return; + } + set({ previewMode: { status: 'loading' } }); try { const url = await fetchPreview(zpl, state.label); // Avoid clobbering an exit that happened while the request was in @@ -851,6 +894,7 @@ export const useLabelStore = create()( URL.revokeObjectURL(url); return; } + previewCache.set(zpl, url); set({ previewMode: { status: 'active', url } }); } catch (e) { if (get().previewMode.status !== 'loading') return; @@ -860,9 +904,8 @@ export const useLabelStore = create()( exitPreviewMode: () => set((state) => { - if (state.previewMode.status === 'active') { - URL.revokeObjectURL(state.previewMode.url); - } + // The blob URL is owned by `previewCache` and intentionally + // kept alive across exits so a re-toggle skips the fetch. if (state.previewMode.status === 'idle') return {}; return { previewMode: { status: 'idle' } }; }), From 4b47dfdf6e8061e55e3a6ff170f4cd6197a4abe8 Mon Sep 17 00:00:00 2001 From: u8array Date: Sat, 16 May 2026 09:54:44 +0200 Subject: [PATCH 2/3] fix(preview): guard cache writes against stale in-flight fetches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gemini's PR review flagged a race the audit missed: if the user opens preview, exits while the fetch is still in flight, edits the design, and re-opens preview, two fetches end up overlapping. The first one resolves while `status === 'loading'` (which it is again, for the new request) and the old check let it through — clobbering the new loading state with the previous design's URL and caching a stale pair. Add a `zpl !== currentZpl` check next to the status check, gated through one `isStale()` helper used by both the success and the error paths. Captured `zpl` in the closure is the ground truth for which request this continuation belongs to. Regression test stages the exact scenario with hand-controlled fetchPreview promises: enter → exit → mutate → enter → resolve the first (stale) → assert state stays `loading` and the stale blob got revoked → resolve the second → assert the fresh URL is the one that lands in state. --- src/store/labelStore.test.ts | 44 ++++++++++++++++++++++++++++++++++++ src/store/labelStore.ts | 16 +++++++++---- 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/src/store/labelStore.test.ts b/src/store/labelStore.test.ts index 10297c1f..b3a507c1 100644 --- a/src/store/labelStore.test.ts +++ b/src/store/labelStore.test.ts @@ -995,6 +995,50 @@ describe('ungroup', () => { expect(activeUrl()).toBe(firstUrl); }); + it('discards a stale in-flight fetch when the user exits, edits, and re-enters', async () => { + // Scenario: user opens preview (fetch 1 in flight), exits, mutates + // design, opens preview again (fetch 2 in flight). Without the + // stale-fetch guard, fetch 1 resolves while status is `loading` + // (from fetch 2) and would overwrite state with the previous + // design's URL. + const labelary = await import('../lib/labelary'); + const fetchSpy = vi.mocked(labelary.fetchPreview); + fetchSpy.mockClear(); + + // Hand-controlled promises so we can interleave the two fetches. + type Resolver = (url: string) => void; + const noop: Resolver = () => undefined; + let resolveFirst: Resolver = noop; + let resolveSecond: Resolver = noop; + fetchSpy.mockImplementationOnce( + () => new Promise((r) => (resolveFirst = r)), + ); + fetchSpy.mockImplementationOnce( + () => new Promise((r) => (resolveSecond = r)), + ); + + const first = state().enterPreviewMode(); + state().exitPreviewMode(); + state().addObject('text'); // changes the ZPL + const second = state().enterPreviewMode(); + + resolveFirst('blob:stale-1'); + await first; + + // Status must still be `loading` for fetch 2 — fetch 1's URL was + // for the previous design and must have been discarded + revoked. + expect(state().previewMode.status).toBe('loading'); + expect(vi.mocked(globalThis.URL.revokeObjectURL)).toHaveBeenCalledWith( + 'blob:stale-1', + ); + + resolveSecond('blob:fresh-2'); + await second; + + expect(state().previewMode.status).toBe('active'); + expect(activeUrl()).toBe('blob:fresh-2'); + }); + it('re-fetches and revokes the stale URL when the ZPL changes', async () => { const labelary = await import('../lib/labelary'); const fetchSpy = vi.mocked(labelary.fetchPreview); diff --git a/src/store/labelStore.ts b/src/store/labelStore.ts index 8e35846a..ecaa3398 100644 --- a/src/store/labelStore.ts +++ b/src/store/labelStore.ts @@ -886,18 +886,26 @@ export const useLabelStore = create()( return; } set({ previewMode: { status: 'loading' } }); + // Two checks guard against settling a stale request: the status + // check catches an exit that happened during the fetch; the ZPL + // recheck catches the harder case where the user exited AND + // re-entered with a different design (so status is `loading` + // again — but for a different request whose result we mustn't + // overwrite). The captured `zpl` is the key for this request; + // anything else means the result no longer matches the state. + const isStale = (): boolean => + get().previewMode.status !== 'loading' || + generateZPL(get().label, currentObjects(get())) !== zpl; try { const url = await fetchPreview(zpl, state.label); - // Avoid clobbering an exit that happened while the request was in - // flight — if the user toggled off, the loading state is gone. - if (get().previewMode.status !== 'loading') { + if (isStale()) { URL.revokeObjectURL(url); return; } previewCache.set(zpl, url); set({ previewMode: { status: 'active', url } }); } catch (e) { - if (get().previewMode.status !== 'loading') return; + if (isStale()) return; set({ previewMode: { status: 'error', error: labelaryErrorMessage(e) } }); } }, From a6299cfb30be9380b77249f02a4882934b99a51d Mon Sep 17 00:00:00 2001 From: u8array Date: Sat, 16 May 2026 10:05:33 +0200 Subject: [PATCH 3/3] perf(preview): use reference equality in isStale, not full ZPL regen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gemini PR feedback: regenerating the ZPL string each time a fetch resolves is wasted work when the store already mutates `label` and the objects array immutably — a reference compare is O(1) versus O(generator pass + coordinate conversions) and detects the exact same "state diverged while we were fetching" condition. As a bonus, the reference compare catches a page switch (which yields a different `objects` array) even when both pages happen to generate identical ZPL by coincidence; the previous string-based check would let that through. --- src/store/labelStore.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/store/labelStore.ts b/src/store/labelStore.ts index ecaa3398..d2ad6a60 100644 --- a/src/store/labelStore.ts +++ b/src/store/labelStore.ts @@ -887,15 +887,18 @@ export const useLabelStore = create()( } set({ previewMode: { status: 'loading' } }); // Two checks guard against settling a stale request: the status - // check catches an exit that happened during the fetch; the ZPL - // recheck catches the harder case where the user exited AND - // re-entered with a different design (so status is `loading` - // again — but for a different request whose result we mustn't - // overwrite). The captured `zpl` is the key for this request; - // anything else means the result no longer matches the state. + // check catches an exit that happened during the fetch; the + // reference-equality check catches the harder case where the + // user exited AND re-entered with a different design (so status + // is `loading` again — but for a different request whose result + // we mustn't overwrite). The store mutates label and objects + // immutably, so a reference change is the cheapest, most + // precise way to detect a divergent state — no string rebuild + // needed, and a page switch is caught too (different array). const isStale = (): boolean => get().previewMode.status !== 'loading' || - generateZPL(get().label, currentObjects(get())) !== zpl; + get().label !== state.label || + currentObjects(get()) !== objs; try { const url = await fetchPreview(zpl, state.label); if (isStale()) {