Merge pull request #1988 from mermaid-js/sidv/runes-migration-fixes

fix: Address regressions and structural risks in the runes migration

src/lib/components/MobileEditor.svelte

@@ -15,7 +15,10 @@
 
   let editorView: EditorView | undefined;
   let editorContainer: HTMLDivElement;
-  let currentText = $state('');
+  // Deliberately not $state: the sync effect below both reads and writes it,
+  // so a reactive currentText would make every keystroke re-run the effect
+  // against the not-yet-revalidated state and revert the user's input.
+  let currentText = '';
   const themeCompartment = new Compartment();
   const languageCompartment = new Compartment();
 

src/lib/util/persist.svelte.test.ts

@@ -22,6 +22,12 @@ describe('readJSON', () => {
     window.localStorage.setItem('legacy', 'undefined');
     expect(readJSON('legacy', 'fallback')).toBe('fallback');
   });
+
+  it('returns the fallback when the stored value parses to null', () => {
+    // The pre-runes persistence layer treated a stored null as absent.
+    window.localStorage.setItem('legacy-null', 'null');
+    expect(readJSON('legacy-null', 'fallback')).toBe('fallback');
+  });
 });
 
 describe('writeJSON', () => {
@@ -51,4 +57,10 @@ describe('persisted', () => {
     expect(counter.value).toBe(42);
     expect(window.localStorage.getItem('counter')).toBe('42');
   });
+
+  it('uses the initial value when storage holds a literal null', () => {
+    window.localStorage.setItem('settings', 'null');
+    const settings = persisted('settings', { theme: 'default' });
+    expect(settings.value).toEqual({ theme: 'default' });
+  });
 });

src/lib/util/persist.svelte.ts

@@ -15,7 +15,12 @@ export const readJSON = <T>(key: string, fallback: T): T => {
   }
   try {
     const raw = window.localStorage.getItem(key);
-    return raw === null ? fallback : (JSON.parse(raw) as T);
+    if (raw === null) {
+      return fallback;
+    }
+    // A stored literal "null" means the value is absent: the pre-runes
+    // persistence layer never wrote null and treated it as missing.
+    return (JSON.parse(raw) as T) ?? fallback;
   } catch {
     return fallback;
   }
@@ -32,8 +37,10 @@ export interface Persisted<T> {
 }
 
 // A localStorage-backed reactive value. Reads on init, writes on every set.
+// Raw state: replace `value` wholesale to change it. With a deep proxy,
+// in-place mutation would update the UI without ever being persisted.
 export const persisted = <T>(key: string, initial: T): Persisted<T> => {
-  let value = $state<T>(readJSON(key, initial));
+  let value = $state.raw<T>(readJSON(key, initial));
   return {
     get value() {
       return value;

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);
+  });
+});

src/lib/util/state.svelte.ts

@@ -42,32 +42,35 @@ const urlParseFailedState = `flowchart TD
     G --> |"No :("| H(Try using the Timeline tab in History <br/>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<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<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<State> = input;
+
+const validatedStateOf = (state: State, serialized: string): ValidatedState => ({
   ...state,
   editorMode: state.editorMode ?? 'code',
   error: undefined,
   errorMarkers: [],
-  serialized: serializeState(state)
+  serialized
 });
 
-let validatedCurrent = $state<ValidatedState>(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<ValidatedState>(
+  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<State>): void => {
+const applyPartial = (state: State, newState: Partial<State>): void => {
   renderCount++;
-  Object.assign(inputState, newState, { renderCount });
-  persistAndProcess();
+  Object.assign(state, newState, { renderCount });
+};
+
+export const updateCodeStore = (newState: Partial<State>): 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<string, unknown>)[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<string, unknown>)[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 }));
 };

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