fix(comparisonStore): preserve stored selection on cold load
The seed-defaults effect fired whenever fontA/fontB were still undefined, including the window between constructor reading storage and the per-id batch resolving. On a slow batch or fast catalog the effect clobbered storage with catalog[0]/catalog[N-1], losing the user's pick on reload. Now bails when storage already holds IDs, and reads storage via untrack so per-font selection writes don't re-trigger the effect. Adds a deterministic regression test that controls catalog/batch ordering via mockImplementation timing.
This commit is contained in:
@@ -102,7 +102,7 @@ export class ComparisonStore {
|
|||||||
this.#fontsByIdsStore = new FontsByIdsStore(fontAId && fontBId ? [fontAId, fontBId] : []);
|
this.#fontsByIdsStore = new FontsByIdsStore(fontAId && fontBId ? [fontAId, fontBId] : []);
|
||||||
|
|
||||||
$effect.root(() => {
|
$effect.root(() => {
|
||||||
// Effect 1: Sync batch results → fontA / fontB
|
// Sync batch results → fontA / fontB
|
||||||
$effect(() => {
|
$effect(() => {
|
||||||
const fonts = this.#fontsByIdsStore.fonts;
|
const fonts = this.#fontsByIdsStore.fonts;
|
||||||
if (fonts.length === 0) {
|
if (fonts.length === 0) {
|
||||||
@@ -124,7 +124,7 @@ export class ComparisonStore {
|
|||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
// Effect 2: Trigger font loading whenever selection or weight changes
|
// Trigger font loading whenever selection or weight changes
|
||||||
$effect(() => {
|
$effect(() => {
|
||||||
const fa = this.#fontA;
|
const fa = this.#fontA;
|
||||||
const fb = this.#fontB;
|
const fb = this.#fontB;
|
||||||
@@ -154,24 +154,38 @@ export class ComparisonStore {
|
|||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
// Effect 3: Set default fonts when storage is empty
|
// Set default fonts when storage is empty
|
||||||
$effect(() => {
|
$effect(() => {
|
||||||
if (this.#fontA && this.#fontB) {
|
if (this.#fontA && this.#fontB) {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Don't clobber a pending rehydration - only seed when storage is empty.
|
||||||
|
// Untracked: only the catalog load should drive this effect, not the
|
||||||
|
// user's storage writes that happen as a result of normal selection.
|
||||||
|
const hasStoredSelection = untrack(() => {
|
||||||
|
return storage.value.fontAId !== null || storage.value.fontBId !== null;
|
||||||
|
});
|
||||||
|
|
||||||
|
if (hasStoredSelection) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
const fonts = fontCatalogStore.fonts;
|
const fonts = fontCatalogStore.fonts;
|
||||||
if (fonts.length >= 2) {
|
|
||||||
|
if (fonts.length < 2) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
untrack(() => {
|
untrack(() => {
|
||||||
const id1 = fonts[0].id;
|
const id1 = fonts[0].id;
|
||||||
const id2 = fonts[fonts.length - 1].id;
|
const id2 = fonts[fonts.length - 1].id;
|
||||||
storage.value = { fontAId: id1, fontBId: id2 };
|
storage.value = { fontAId: id1, fontBId: id2 };
|
||||||
this.#fontsByIdsStore.setIds([id1, id2]);
|
this.#fontsByIdsStore.setIds([id1, id2]);
|
||||||
});
|
});
|
||||||
}
|
|
||||||
});
|
});
|
||||||
|
|
||||||
// Effect 4: Pin fontA/fontB so eviction never removes on-screen fonts
|
// Pin fontA/fontB so eviction never removes on-screen fonts
|
||||||
$effect(() => {
|
$effect(() => {
|
||||||
const fa = this.#fontA;
|
const fa = this.#fontA;
|
||||||
const fb = this.#fontB;
|
const fb = this.#fontB;
|
||||||
|
|||||||
@@ -165,6 +165,46 @@ describe('ComparisonStore', () => {
|
|||||||
expect(mockStorage._value.fontBId).toBe(mockFontB.id);
|
expect(mockStorage._value.fontBId).toBe(mockFontB.id);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Regression: when storage already holds the user's selection, the
|
||||||
|
* seed-defaults effect must bail out — even if it fires before the
|
||||||
|
* per-id batch returns (catalog wins the race on slow networks or
|
||||||
|
* cold reloads). Pre-fix the effect only checked fontA/fontB, both
|
||||||
|
* still undefined at this point, and clobbered storage with whatever
|
||||||
|
* the catalog had as fonts[0] / fonts[N-1].
|
||||||
|
*/
|
||||||
|
it('should not overwrite stored IDs when batch is still in flight', async () => {
|
||||||
|
const seededA = UNIFIED_FONTS.lato;
|
||||||
|
const seededB = UNIFIED_FONTS.montserrat;
|
||||||
|
|
||||||
|
mockStorage._value.fontAId = seededA.id;
|
||||||
|
mockStorage._value.fontBId = seededB.id;
|
||||||
|
|
||||||
|
// Catalog defaults differ from the stored selection — if the
|
||||||
|
// effect mis-seeds, storage will flip to roboto / open-sans.
|
||||||
|
(fontCatalogStore as any).fonts = [mockFontA, mockFontB];
|
||||||
|
|
||||||
|
// Delay the batch so the catalog-driven effect runs first.
|
||||||
|
vi.spyOn(proxyFonts, 'fetchFontsByIds').mockImplementation(
|
||||||
|
() => new Promise(r => setTimeout(() => r([seededA, seededB]), 50)),
|
||||||
|
);
|
||||||
|
|
||||||
|
const store = new ComparisonStore();
|
||||||
|
|
||||||
|
// Let the catalog effect run; storage must be untouched.
|
||||||
|
await new Promise(r => setTimeout(r, 10));
|
||||||
|
expect(mockStorage._value.fontAId).toBe(seededA.id);
|
||||||
|
expect(mockStorage._value.fontBId).toBe(seededB.id);
|
||||||
|
|
||||||
|
// Batch resolves with the seeded selection — fontA/B must match.
|
||||||
|
await vi.waitFor(() => {
|
||||||
|
expect(store.fontA?.id).toBe(seededA.id);
|
||||||
|
expect(store.fontB?.id).toBe(seededB.id);
|
||||||
|
}, { timeout: 2000 });
|
||||||
|
expect(mockStorage._value.fontAId).toBe(seededA.id);
|
||||||
|
expect(mockStorage._value.fontBId).toBe(seededB.id);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('Aggregate Loading State', () => {
|
describe('Aggregate Loading State', () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user