From 0aae710e35c0ad505434671f94b2660bed3c69cc Mon Sep 17 00:00:00 2001 From: Ilia Mashkov Date: Wed, 3 Jun 2026 10:16:47 +0300 Subject: [PATCH] fix: dispose persistent-store effects; close comparisonStore effect leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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(). --- .../typographySettingsStore.svelte.ts | 1 + .../store/ThemeManager/ThemeManager.svelte.ts | 1 + src/shared/lib/index.ts | 2 + .../model/stores/comparisonStore.svelte.ts | 63 ++++++++++++------- .../model/stores/comparisonStore.test.ts | 2 + .../stores/layoutStore/layoutStore.svelte.ts | 7 +++ 6 files changed, 53 insertions(+), 23 deletions(-) diff --git a/src/features/AdjustTypography/model/store/typographySettingsStore/typographySettingsStore.svelte.ts b/src/features/AdjustTypography/model/store/typographySettingsStore/typographySettingsStore.svelte.ts index e0c4224..9960c9f 100644 --- a/src/features/AdjustTypography/model/store/typographySettingsStore/typographySettingsStore.svelte.ts +++ b/src/features/AdjustTypography/model/store/typographySettingsStore/typographySettingsStore.svelte.ts @@ -166,6 +166,7 @@ export class TypographySettingsStore { */ destroy(): void { this.#disposeEffects(); + this.#storage.destroy(); } /** diff --git a/src/features/ChangeAppTheme/model/store/ThemeManager/ThemeManager.svelte.ts b/src/features/ChangeAppTheme/model/store/ThemeManager/ThemeManager.svelte.ts index 271063f..78ba77d 100644 --- a/src/features/ChangeAppTheme/model/store/ThemeManager/ThemeManager.svelte.ts +++ b/src/features/ChangeAppTheme/model/store/ThemeManager/ThemeManager.svelte.ts @@ -125,6 +125,7 @@ class ThemeManager { destroy(): void { this.#mediaQuery?.removeEventListener('change', this.#systemChangeHandler); this.#mediaQuery = null; + this.#store.destroy(); } /** diff --git a/src/shared/lib/index.ts b/src/shared/lib/index.ts index 0e52af8..918a4d5 100644 --- a/src/shared/lib/index.ts +++ b/src/shared/lib/index.ts @@ -11,6 +11,7 @@ export { createPersistentStore, createPerspectiveManager, createResponsiveManager, + createSingleton, createVirtualizer, type Entity, type EntityStore, @@ -21,6 +22,7 @@ export { type Property, type ResponsiveManager, responsiveManager, + type Singleton, type VirtualItem, type Virtualizer, type VirtualizerOptions, diff --git a/src/widgets/ComparisonView/model/stores/comparisonStore.svelte.ts b/src/widgets/ComparisonView/model/stores/comparisonStore.svelte.ts index 1f2bb28..546e221 100644 --- a/src/widgets/ComparisonView/model/stores/comparisonStore.svelte.ts +++ b/src/widgets/ComparisonView/model/stores/comparisonStore.svelte.ts @@ -27,7 +27,10 @@ import { type TypographySettingsStore, getTypographySettingsStore, } from '$features/AdjustTypography'; -import { createPersistentStore } from '$shared/lib'; +import { + createPersistentStore, + createSingleton, +} from '$shared/lib'; import { untrack } from 'svelte'; import { getPretextFontString } from '../../lib'; @@ -56,12 +59,6 @@ const STORAGE_KEY = 'glyphdiff:comparison'; */ const FONT_READY_FALLBACK_MS = 1000; -// Persistent storage for selected comparison fonts -const storage = createPersistentStore(STORAGE_KEY, { - fontAId: null, - fontBId: null, -}); - /** * Store for managing font comparison state. * @@ -106,16 +103,27 @@ export class ComparisonStore { #typography: TypographySettingsStore; #lifecycle: FontLifecycleManager; + /** + * Per-instance persistent storage for the selected comparison fonts. + */ + #storage = createPersistentStore(STORAGE_KEY, { + fontAId: null, + fontBId: null, + }); + /** + * Disposes the constructor's $effect.root. Must be run on teardown. + */ + #disposeEffects: () => void; constructor() { // 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.#fontCatalog = getFontCatalog(); this.#typography = getTypographySettingsStore(); this.#lifecycle = getFontLifecycleManager(); - $effect.root(() => { + this.#disposeEffects = $effect.root(() => { // Sync batch results → fontA / fontB $effect(() => { const fonts = this.#fontsByIdsStore.fonts; @@ -123,7 +131,7 @@ export class ComparisonStore { return; } - const { fontAId: aId, fontBId: bId } = storage.value; + const { fontAId: aId, fontBId: bId } = this.#storage.value; if (aId) { const fa = fonts.find(f => f.id === aId); if (fa) { @@ -178,7 +186,7 @@ export class ComparisonStore { // 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; + return this.#storage.value.fontAId !== null || this.#storage.value.fontBId !== null; }); if (hasStoredSelection) { @@ -194,7 +202,7 @@ export class ComparisonStore { untrack(() => { const id1 = fonts[0].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]); }); }); @@ -279,7 +287,7 @@ export class ComparisonStore { * Updates persistent storage with the current font selection. */ private updateStorage() { - storage.value = { + this.#storage.value = { fontAId: this.#fontA?.id ?? null, fontBId: this.#fontB?.id ?? null, }; @@ -363,19 +371,28 @@ export class ComparisonStore { this.#fontA = undefined; this.#fontB = undefined; this.#fontsByIdsStore.setIds([]); - storage.clear(); + this.#storage.clear(); 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 { - return (_comparisonStore ??= new ComparisonStore()); -} +export const getComparisonStore = comparisonStore.get; -// test-only reset, so specs don't share a live observer -export function __resetComparisonStore() { - _comparisonStore?.resetAll(); - _comparisonStore = undefined; -} +// test-only reset, so specs don't share a live observer or persisted state +export const __resetComparisonStore = comparisonStore.reset; diff --git a/src/widgets/ComparisonView/model/stores/comparisonStore.test.ts b/src/widgets/ComparisonView/model/stores/comparisonStore.test.ts index 021193a..1132c65 100644 --- a/src/widgets/ComparisonView/model/stores/comparisonStore.test.ts +++ b/src/widgets/ComparisonView/model/stores/comparisonStore.test.ts @@ -46,6 +46,8 @@ const mockStorage = vi.hoisted(() => { configurable: true, }); + storage.destroy = vi.fn(); + return storage; }); diff --git a/src/widgets/SampleList/model/stores/layoutStore/layoutStore.svelte.ts b/src/widgets/SampleList/model/stores/layoutStore/layoutStore.svelte.ts index b40a19c..f9c8189 100644 --- a/src/widgets/SampleList/model/stores/layoutStore/layoutStore.svelte.ts +++ b/src/widgets/SampleList/model/stores/layoutStore/layoutStore.svelte.ts @@ -144,6 +144,13 @@ class LayoutManager { this.#mode = DEFAULT_CONFIG.mode; this.#store.clear(); } + + /** + * Dispose the persistent store's save effect. Call on store disposal. + */ + destroy(): void { + this.#store.destroy(); + } } let _layoutManager: LayoutManager | undefined;