fix: dispose persistent-store effects; close comparisonStore effect leak

Thread the new createPersistentStore.destroy() through its owners so the
save effect.root is actually torn down: LayoutManager gains destroy();
ThemeManager and typographySettings dispose their #store in their existing
destroy().

comparisonStore had its own leak — a constructor $effect.root whose disposer
was discarded, and a module-level persistent storage created at import. Move
storage into the instance (#storage, created lazily per instance), capture
the effect.root disposer, add destroy(), and adopt createSingleton so reset
runs resetAll() + destroy(). Re-export createSingleton from the $shared/lib
barrel; give the test storage mock a destroy().
This commit is contained in:
Ilia Mashkov
2026-06-03 10:16:47 +03:00
parent ded9606c30
commit 0aae710e35
6 changed files with 53 additions and 23 deletions
@@ -166,6 +166,7 @@ export class TypographySettingsStore {
*/ */
destroy(): void { destroy(): void {
this.#disposeEffects(); this.#disposeEffects();
this.#storage.destroy();
} }
/** /**
@@ -125,6 +125,7 @@ class ThemeManager {
destroy(): void { destroy(): void {
this.#mediaQuery?.removeEventListener('change', this.#systemChangeHandler); this.#mediaQuery?.removeEventListener('change', this.#systemChangeHandler);
this.#mediaQuery = null; this.#mediaQuery = null;
this.#store.destroy();
} }
/** /**
+2
View File
@@ -11,6 +11,7 @@ export {
createPersistentStore, createPersistentStore,
createPerspectiveManager, createPerspectiveManager,
createResponsiveManager, createResponsiveManager,
createSingleton,
createVirtualizer, createVirtualizer,
type Entity, type Entity,
type EntityStore, type EntityStore,
@@ -21,6 +22,7 @@ export {
type Property, type Property,
type ResponsiveManager, type ResponsiveManager,
responsiveManager, responsiveManager,
type Singleton,
type VirtualItem, type VirtualItem,
type Virtualizer, type Virtualizer,
type VirtualizerOptions, type VirtualizerOptions,
@@ -27,7 +27,10 @@ import {
type TypographySettingsStore, type TypographySettingsStore,
getTypographySettingsStore, getTypographySettingsStore,
} from '$features/AdjustTypography'; } from '$features/AdjustTypography';
import { createPersistentStore } from '$shared/lib'; import {
createPersistentStore,
createSingleton,
} from '$shared/lib';
import { untrack } from 'svelte'; import { untrack } from 'svelte';
import { getPretextFontString } from '../../lib'; import { getPretextFontString } from '../../lib';
@@ -56,12 +59,6 @@ const STORAGE_KEY = 'glyphdiff:comparison';
*/ */
const FONT_READY_FALLBACK_MS = 1000; const FONT_READY_FALLBACK_MS = 1000;
// Persistent storage for selected comparison fonts
const storage = createPersistentStore<ComparisonState>(STORAGE_KEY, {
fontAId: null,
fontBId: null,
});
/** /**
* Store for managing font comparison state. * Store for managing font comparison state.
* *
@@ -106,16 +103,27 @@ export class ComparisonStore {
#typography: TypographySettingsStore; #typography: TypographySettingsStore;
#lifecycle: FontLifecycleManager; #lifecycle: FontLifecycleManager;
/**
* Per-instance persistent storage for the selected comparison fonts.
*/
#storage = createPersistentStore<ComparisonState>(STORAGE_KEY, {
fontAId: null,
fontBId: null,
});
/**
* Disposes the constructor's $effect.root. Must be run on teardown.
*/
#disposeEffects: () => void;
constructor() { constructor() {
// Synchronously seed the batch store with any IDs already in storage // Synchronously seed the batch store with any IDs already in storage
const { fontAId, fontBId } = storage.value; const { fontAId, fontBId } = this.#storage.value;
this.#fontsByIdsStore = new FontsByIdsStore(fontAId && fontBId ? [fontAId, fontBId] : []); this.#fontsByIdsStore = new FontsByIdsStore(fontAId && fontBId ? [fontAId, fontBId] : []);
this.#fontCatalog = getFontCatalog(); this.#fontCatalog = getFontCatalog();
this.#typography = getTypographySettingsStore(); this.#typography = getTypographySettingsStore();
this.#lifecycle = getFontLifecycleManager(); this.#lifecycle = getFontLifecycleManager();
$effect.root(() => { this.#disposeEffects = $effect.root(() => {
// Sync batch results → fontA / fontB // Sync batch results → fontA / fontB
$effect(() => { $effect(() => {
const fonts = this.#fontsByIdsStore.fonts; const fonts = this.#fontsByIdsStore.fonts;
@@ -123,7 +131,7 @@ export class ComparisonStore {
return; return;
} }
const { fontAId: aId, fontBId: bId } = storage.value; const { fontAId: aId, fontBId: bId } = this.#storage.value;
if (aId) { if (aId) {
const fa = fonts.find(f => f.id === aId); const fa = fonts.find(f => f.id === aId);
if (fa) { if (fa) {
@@ -178,7 +186,7 @@ export class ComparisonStore {
// Untracked: only the catalog load should drive this effect, not the // Untracked: only the catalog load should drive this effect, not the
// user's storage writes that happen as a result of normal selection. // user's storage writes that happen as a result of normal selection.
const hasStoredSelection = untrack(() => { const hasStoredSelection = untrack(() => {
return storage.value.fontAId !== null || storage.value.fontBId !== null; return this.#storage.value.fontAId !== null || this.#storage.value.fontBId !== null;
}); });
if (hasStoredSelection) { if (hasStoredSelection) {
@@ -194,7 +202,7 @@ export class ComparisonStore {
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 }; this.#storage.value = { fontAId: id1, fontBId: id2 };
this.#fontsByIdsStore.setIds([id1, id2]); this.#fontsByIdsStore.setIds([id1, id2]);
}); });
}); });
@@ -279,7 +287,7 @@ export class ComparisonStore {
* Updates persistent storage with the current font selection. * Updates persistent storage with the current font selection.
*/ */
private updateStorage() { private updateStorage() {
storage.value = { this.#storage.value = {
fontAId: this.#fontA?.id ?? null, fontAId: this.#fontA?.id ?? null,
fontBId: this.#fontB?.id ?? null, fontBId: this.#fontB?.id ?? null,
}; };
@@ -363,19 +371,28 @@ export class ComparisonStore {
this.#fontA = undefined; this.#fontA = undefined;
this.#fontB = undefined; this.#fontB = undefined;
this.#fontsByIdsStore.setIds([]); this.#fontsByIdsStore.setIds([]);
storage.clear(); this.#storage.clear();
this.#typography.reset(); this.#typography.reset();
} }
/**
* Disposes reactive effects and the persistent store. Call on teardown.
*/
destroy() {
this.#disposeEffects();
this.#storage.destroy();
}
} }
let _comparisonStore: ComparisonStore | undefined; const comparisonStore = createSingleton(
() => new ComparisonStore(),
instance => {
instance.resetAll();
instance.destroy();
},
);
export function getComparisonStore(): ComparisonStore { export const getComparisonStore = comparisonStore.get;
return (_comparisonStore ??= new ComparisonStore());
}
// test-only reset, so specs don't share a live observer // test-only reset, so specs don't share a live observer or persisted state
export function __resetComparisonStore() { export const __resetComparisonStore = comparisonStore.reset;
_comparisonStore?.resetAll();
_comparisonStore = undefined;
}
@@ -46,6 +46,8 @@ const mockStorage = vi.hoisted(() => {
configurable: true, configurable: true,
}); });
storage.destroy = vi.fn();
return storage; return storage;
}); });
@@ -144,6 +144,13 @@ class LayoutManager {
this.#mode = DEFAULT_CONFIG.mode; this.#mode = DEFAULT_CONFIG.mode;
this.#store.clear(); this.#store.clear();
} }
/**
* Dispose the persistent store's save effect. Call on store disposal.
*/
destroy(): void {
this.#store.destroy();
}
} }
let _layoutManager: LayoutManager | undefined; let _layoutManager: LayoutManager | undefined;