From d2f067a540f9aeb553670997287680a666117565 Mon Sep 17 00:00:00 2001 From: Sidharth Vinod Date: Wed, 10 Jun 2026 23:57:01 +0530 Subject: [PATCH] refactor: Route every input state mutation through one untracked gateway MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The untrack()/persistAndProcess() pair was a per-function convention: each update function had to remember both, a forgotten untrack would let calling effects subscribe to input state (the bug fixed in 6a9a306e), and a forgotten persistAndProcess would silently skip persistence and re-validation. A single update(mutate) gateway now makes both structural, and the scattered untrack calls (three of which were dead) are gone. Also: - Export inputState as Readonly so writes outside the update functions are type errors instead of comment violations. - Share the 'codeStore' key as a constant instead of two literals. - Reuse validatedStateOf() in processState instead of duplicating the validated-state defaults. - Use $state.raw for validatedCurrent: it is only ever replaced wholesale, so deep proxying every published state was pure overhead. The new state.svelte.test.ts pins the invariants: update functions never make a calling effect track input state, and every mutation persists. Vitest needed resolve.conditions=['browser'] for those tests — it was loading Svelte's server build, where $effect is a no-op. Co-Authored-By: Claude Fable 5 --- src/lib/util/state.svelte.test.ts | 88 ++++++++++++++++++++ src/lib/util/state.svelte.ts | 132 +++++++++++++++++------------- vite.config.js | 2 + 3 files changed, 167 insertions(+), 55 deletions(-) create mode 100644 src/lib/util/state.svelte.test.ts diff --git a/src/lib/util/state.svelte.test.ts b/src/lib/util/state.svelte.test.ts new file mode 100644 index 00000000..4a3ec35a --- /dev/null +++ b/src/lib/util/state.svelte.test.ts @@ -0,0 +1,88 @@ +import type { State } from '$lib/types'; +import { flushSync } from 'svelte'; +import { describe, expect, it } from 'vitest'; +import { + defaultState, + inputState, + loadState, + replaceInputState, + toggleDarkTheme, + updateCode, + updateCodeStore, + updateConfig, + verifyState +} from './state.svelte'; + +// Runs `body` inside an effect and reports how often the effect (re-)runs. +const countEffectRuns = (body: () => void): { runs: () => number; stop: () => void } => { + let runs = 0; + const stop = $effect.root(() => { + $effect(() => { + runs++; + body(); + }); + }); + flushSync(); + return { runs: () => runs, stop }; +}; + +const readStoredState = (): State => + JSON.parse(window.localStorage.getItem('codeStore') ?? '{}') as State; + +describe('update functions called from effects', () => { + // Effects that call an update function must not subscribe to the input + // state the function reads, or unrelated state changes re-fire the effect + // (and self-reads loop, e.g. the dark-theme effect in +layout.svelte). + const cases: [string, () => void][] = [ + ['updateCodeStore', () => updateCodeStore({})], + ['updateCode', () => updateCode('graph TD\n inside-effect')], + ['updateConfig', () => updateConfig('{"theme":"default"}')], + ['toggleDarkTheme', () => toggleDarkTheme(false)], + ['replaceInputState', () => replaceInputState({ ...defaultState })], + ['verifyState', () => verifyState()], + ['loadState', () => loadState('')] + ]; + + it.each(cases)('%s does not make the calling effect track input state', (_name, call) => { + const counter = countEffectRuns(call); + try { + expect(counter.runs()).toBe(1); + updateCode('graph TD\n external-change'); + updateConfig('{"theme":"forest"}'); + updateCodeStore({ pan: { x: 1, y: 2 } }); + flushSync(); + expect(counter.runs()).toBe(1); + } finally { + counter.stop(); + } + }); +}); + +describe('update functions persist input state', () => { + it('updateCode writes the new code to localStorage', () => { + updateCode('graph TD\n persisted-by-test'); + expect(readStoredState().code).toBe('graph TD\n persisted-by-test'); + }); + + it('updateCodeStore merges partial state and persists it', () => { + updateCodeStore({ rough: true }); + expect(inputState.rough).toBe(true); + expect(readStoredState().rough).toBe(true); + }); + + it('replaceInputState drops keys absent from the next state and persists', () => { + updateCodeStore({ pan: { x: 1, y: 2 } }); + expect(inputState.pan).toEqual({ x: 1, y: 2 }); + replaceInputState({ ...defaultState }); + expect(inputState.pan).toBeUndefined(); + expect(readStoredState().pan).toBeUndefined(); + expect(readStoredState().code).toBe(defaultState.code); + }); + + it('verifyState forces panZoom back on', () => { + updateCodeStore({ panZoom: false }); + verifyState(); + expect(inputState.panZoom).toBe(true); + expect(readStoredState().panZoom).toBe(true); + }); +}); diff --git a/src/lib/util/state.svelte.ts b/src/lib/util/state.svelte.ts index 3fe59457..29913377 100644 --- a/src/lib/util/state.svelte.ts +++ b/src/lib/util/state.svelte.ts @@ -42,32 +42,35 @@ const urlParseFailedState = `flowchart TD G --> |"No :("| H(Try using the Timeline tab in History
from same browser you used to create the diagram.) click D href "https://github.com/mermaid-js/mermaid-live-editor/issues/new?assignees=&labels=bug&template=bug_report.md&title=Broken%20link" "Raise issue"`; -// inputState handles all updates and is shared externally when exporting via URL, History, etc. -// It is reactive for reads; every write must go through the update functions -// below so each change is persisted and re-validated. -// The fallback is cloned so mutations never write through to defaultState. -export const inputState = $state(readJSON('codeStore', { ...defaultState })); +const CODE_STORE_KEY = 'codeStore'; -const validatedStateOf = (state: State): ValidatedState => ({ +// The single mutable input state; only update() below may write to it. +// The fallback is cloned so mutations never write through to defaultState. +const input = $state(readJSON(CODE_STORE_KEY, { ...defaultState })); + +// inputState is shared externally when exporting via URL, History, etc. +// It is reactive for reads; the read-only type keeps writes inside this +// module, where update() persists and re-validates every change. +export const inputState: Readonly = input; + +const validatedStateOf = (state: State, serialized: string): ValidatedState => ({ ...state, editorMode: state.editorMode ?? 'code', error: undefined, errorMarkers: [], - serialized: serializeState(state) + serialized }); -let validatedCurrent = $state(validatedStateOf($state.snapshot(inputState))); +const initialState = $state.snapshot(input) as State; +// Only ever replaced wholesale, so raw (shallow) reactivity is enough. +let validatedCurrent = $state.raw( + validatedStateOf(initialState, serializeState(initialState)) +); let lastDiagramType = ''; const processState = async (state: State) => { - const processed: ValidatedState = { - ...state, - editorMode: state.editorMode ?? 'code', - error: undefined, - errorMarkers: [], - serialized: '' - }; + const processed = validatedStateOf(state, ''); // No changes should be done to fields part of `state`. try { processed.serialized = serializeState(state); @@ -127,17 +130,28 @@ let updateHash: ((serialized: string) => void) | undefined; // Persist the current input state and asynchronously re-validate it, // publishing the result to `validatedState` (and the URL hash, once -// initURLSubscription has run). Reads are untracked so effects that call an -// update function don't start depending on the whole input state. +// initURLSubscription has run). Only called from update(), which suppresses +// dependency tracking. const persistAndProcess = (): void => { - const snapshot = untrack(() => $state.snapshot(inputState)); - writeJSON('codeStore', snapshot); + const snapshot = $state.snapshot(input) as State; + writeJSON(CODE_STORE_KEY, snapshot); void processState(snapshot).then((processed) => { validatedCurrent = processed; updateHash?.(processed.serialized); }); }; +// The single mutation gateway: every update function funnels its writes +// through here. The mutator runs untracked so effects that call an update +// function never subscribe to the input state it reads, and the trailing +// persist + re-validate cannot be forgotten by a new update function. +const update = (mutate: (state: State) => void): void => { + untrack(() => { + mutate(input); + persistAndProcess(); + }); +}; + // All internal reads should be done via validatedState, but it should not be // persisted/shared externally. export const validatedState = { @@ -274,27 +288,32 @@ export const sanitizeConfig = (config: string | MermaidConfig) => { }; export const loadState = (data: string): void => { - let state: State; console.log(`Loading '${data}'`); - try { - state = deserializeState(data); - state.mermaid = sanitizeConfig(state.mermaid || defaultState.mermaid); - } catch (error) { - state = untrack(() => $state.snapshot(inputState)); - if (data) { - console.error('Init error', error); - state.code = urlParseFailedState; - state.mermaid = defaultState.mermaid; + update((state) => { + let next: State; + try { + next = deserializeState(data); + next.mermaid = sanitizeConfig(next.mermaid || defaultState.mermaid); + } catch (error) { + next = $state.snapshot(state) as State; + if (data) { + console.error('Init error', error); + next.code = urlParseFailedState; + next.mermaid = defaultState.mermaid; + } } - } - updateCodeStore(state); + applyPartial(state, next); + }); }; let renderCount = 0; -export const updateCodeStore = (newState: Partial): void => { +const applyPartial = (state: State, newState: Partial): void => { renderCount++; - Object.assign(inputState, newState, { renderCount }); - persistAndProcess(); + Object.assign(state, newState, { renderCount }); +}; + +export const updateCodeStore = (newState: Partial): void => { + update((state) => applyPartial(state, newState)); }; export const updateCode = ( @@ -306,13 +325,14 @@ export const updateCode = ( ): void => { errorDebug(); - if (resetPanZoom) { - inputState.pan = undefined; - inputState.zoom = undefined; - } - inputState.code = code; - inputState.updateDiagram = updateDiagram; - persistAndProcess(); + update((state) => { + if (resetPanZoom) { + state.pan = undefined; + state.zoom = undefined; + } + state.code = code; + state.updateDiagram = updateDiagram; + }); }; export const updateConfig = (config: string): void => { @@ -320,25 +340,27 @@ export const updateConfig = (config: string): void => { }; export const toggleDarkTheme = (dark: boolean): void => { - const config = JSON.parse(untrack(() => inputState.mermaid)) as MermaidConfig; - if (!config.theme || ['dark', 'default'].includes(config.theme)) { - config.theme = dark ? 'dark' : 'default'; - } - inputState.mermaid = formatJSON(config); - persistAndProcess(); + update((state) => { + const config = JSON.parse(state.mermaid) as MermaidConfig; + if (!config.theme || ['dark', 'default'].includes(config.theme)) { + config.theme = dark ? 'dark' : 'default'; + } + state.mermaid = formatJSON(config); + }); }; // Replaces the whole input state (e.g. when restoring a history entry), // dropping keys the next state does not define. export const replaceInputState = (next: State): void => { - for (const key of untrack(() => Object.keys(inputState))) { - if (!(key in next)) { - // eslint-disable-next-line @typescript-eslint/no-dynamic-delete -- full-replace semantics - delete (inputState as unknown as Record)[key]; + update((state) => { + for (const key of Object.keys(state)) { + if (!(key in next)) { + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete -- full-replace semantics + delete (state as unknown as Record)[key]; + } } - } - Object.assign(inputState, next); - persistAndProcess(); + Object.assign(state, next); + }); }; export const initURLSubscription = (): void => { @@ -349,5 +371,5 @@ export const initURLSubscription = (): void => { }; export const verifyState = (): void => { - updateCodeStore(untrack(() => inputState.panZoom) ? {} : { panZoom: true }); + update((state) => applyPartial(state, state.panZoom ? {} : { panZoom: true })); }; diff --git a/vite.config.js b/vite.config.js index 48f853eb..d04a88a2 100644 --- a/vite.config.js +++ b/vite.config.js @@ -33,6 +33,8 @@ export default defineConfig({ envPrefix: 'MERMAID_', server: { port: 3000, host: true }, preview: { port: 3000, host: true }, + // Vitest otherwise resolves Svelte's server build, where $effect is a no-op. + resolve: process.env.VITEST ? { conditions: ['browser'] } : undefined, test: { environment: 'jsdom', // in-source testing