mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-26 22:37:03 +00:00
fix(editor): follow the app's light/dark mode in mermaid diagrams (BUG-3106)
initMermaid passed `theme: 'dark'` unconditionally, so every diagram was drawn on the dark palette whether the app was in light or dark mode. The value predates the mermaid 12 bump, which kept it for continuity — true of the BUMP, but it was never a deliberate choice to ignore the app's mode. The mode read is `data-theme` on <html>. There is no theme store: four components (+layout's initializer, TopBar, Sidebar, YouSheet) each toggle that attribute and mirror it to localStorage['pad-theme'], so the attribute is the only thing all four agree on. ABSENT MEANS DARK — +layout only ever SETS the attribute and leaves it off for the default — so the predicate is `!== 'light'`; `=== 'dark'` would draw the default mode, the common case on a fresh browser, on the wrong palette. A theme change needs both halves: a fresh initialize(), because mermaid caches its config, AND a re-render, because the SVG already on screen has the old palette baked in. A MutationObserver on the attribute drives it, rather than a hook in the togglers, so it covers all four without any of them having to know diagrams exist — and covers the next one too. `layout: 'dagre'` and `look: 'classic'` are untouched. mermaidTheme.ts holds the two decisions worth testing — which theme a mode calls for, and which diagrams a change must redraw. Diagrams showing a syntax error are skipped so a toggle does not flicker an error message the reader is already looking at. Verified in the browser on the served binary, reading the painted fill rather than eyeballing it. Branch build on a private port, one flowchart and one sequence diagram, against main on :7777 as the control: main, light mode node fill rgb(31,32,32) <- the bug: dark palette branch, light node fill rgb(236,236,255), text rgb(51,51,51) branch, dark node fill rgb(31,32,32), text rgb(204,204,204) branch, back light node fill rgb(236,236,255) The SVG element id changes on each branch leg (mermaid mints a random id per render), which is what shows the re-render actually fired; on main the id is unchanged across the toggle, the other half of the same bug. Gates: svelte-check 0 errors, vitest 205 passed across 12 editor files. Three mutants killed and the restore verified byte-identical — always-dark (the original bug), the inverted `=== 'dark'` predicate, and dropping the error-class skip. Claude-Session: https://claude.ai/code/session_01BLg7wiBeSgXvAhWi23mT4Z
This commit is contained in:
xarmian
@@ -26,17 +26,46 @@
|
||||
import CodeBlock from '@tiptap/extension-code-block';
|
||||
import Placeholder from '@tiptap/extension-placeholder';
|
||||
import { copyToClipboard } from '$lib/utils/clipboard';
|
||||
import {
|
||||
mermaidThemeForMode,
|
||||
collectMermaidRerenders,
|
||||
type MermaidTheme,
|
||||
} from './mermaidTheme';
|
||||
|
||||
// Serialized mermaid render queue — mermaid can't handle concurrent renders
|
||||
let mermaidMod: typeof import('mermaid') | null = null;
|
||||
let renderQueue: Promise<void> = Promise.resolve();
|
||||
|
||||
// BUG-3106: the mode read and the redraw set live in mermaidTheme.ts so
|
||||
// both are reachable from a test; the comments there carry the reasoning.
|
||||
function currentMermaidTheme(): MermaidTheme {
|
||||
if (typeof document === 'undefined') return 'dark';
|
||||
return mermaidThemeForMode(document.documentElement);
|
||||
}
|
||||
|
||||
// Watches <html>'s data-theme; armed in onMount, disconnected in onDestroy.
|
||||
let themeObserver: MutationObserver | null = null;
|
||||
|
||||
// The theme the loaded module was last initialized with. mermaid caches
|
||||
// its config, so a theme change needs a fresh initialize() AND a re-render
|
||||
// — the SVG already in the DOM carries the old palette baked in.
|
||||
let appliedMermaidTheme: MermaidTheme | null = null;
|
||||
|
||||
async function initMermaid() {
|
||||
if (!mermaidMod) {
|
||||
mermaidMod = await import('mermaid');
|
||||
}
|
||||
const theme = currentMermaidTheme();
|
||||
if (theme !== appliedMermaidTheme) {
|
||||
mermaidMod.default.initialize({
|
||||
startOnLoad: false,
|
||||
theme: 'dark',
|
||||
// BUG-3106: follows the app's mode. This was an unconditional
|
||||
// 'dark', which drew every diagram on the dark palette in light
|
||||
// mode. The TASK-3090 comment below called it "OUR value, not a
|
||||
// default" and kept it through the 12 bump for continuity —
|
||||
// true of the BUMP, but it was never a deliberate choice to
|
||||
// ignore the app's mode, just a value nobody had revisited.
|
||||
theme,
|
||||
securityLevel: 'strict',
|
||||
fontFamily: 'inherit',
|
||||
// `layout` is the load-bearing one. Read from the shipped
|
||||
@@ -46,9 +75,7 @@
|
||||
// on an upgrade nobody asked them about. `look` is a defensive
|
||||
// pin, not a fix: both versions already default to "classic",
|
||||
// and it is written down so a later default change cannot move
|
||||
// our diagrams silently. (`theme` stays "dark" above for the
|
||||
// same continuity reason — it is OUR value, not a default, and
|
||||
// both versions default to "default".)
|
||||
// our diagrams silently.
|
||||
// This is the site default, not a ceiling: a diagram that opts
|
||||
// into ELK in its own frontmatter still gets ELK.
|
||||
//
|
||||
@@ -61,10 +88,20 @@
|
||||
layout: 'dagre',
|
||||
look: 'classic',
|
||||
});
|
||||
appliedMermaidTheme = theme;
|
||||
}
|
||||
return mermaidMod;
|
||||
}
|
||||
|
||||
// Re-render every diagram currently in this editor on the new palette.
|
||||
// The SVG already in the DOM has the old palette baked in, so a fresh
|
||||
// initialize() alone changes nothing that is already on screen.
|
||||
function rerenderMermaidForTheme(root: HTMLElement) {
|
||||
for (const { source, target } of collectMermaidRerenders(root)) {
|
||||
queueMermaidRender(source, target);
|
||||
}
|
||||
}
|
||||
|
||||
function queueMermaidRender(source: string, target: HTMLElement) {
|
||||
renderQueue = renderQueue.then(async () => {
|
||||
try {
|
||||
@@ -883,6 +920,23 @@
|
||||
onMount(() => {
|
||||
if (!element) return;
|
||||
|
||||
// BUG-3106: watch the app's mode so diagrams follow it.
|
||||
//
|
||||
// A MutationObserver on <html>'s data-theme rather than a hook in the
|
||||
// togglers, because there are FOUR of them (+layout's initializer,
|
||||
// TopBar, Sidebar, YouSheet) and they share no store — only the
|
||||
// attribute. Observing the attribute covers all four, and covers the
|
||||
// next one without it having to know diagrams exist.
|
||||
const themeRoot = document.documentElement;
|
||||
themeObserver = new MutationObserver(() => {
|
||||
if (currentMermaidTheme() === appliedMermaidTheme) return;
|
||||
if (element) rerenderMermaidForTheme(element);
|
||||
});
|
||||
themeObserver.observe(themeRoot, {
|
||||
attributes: true,
|
||||
attributeFilter: ['data-theme'],
|
||||
});
|
||||
|
||||
// Resolve the workspace slug at mount time. The Editor lives inside
|
||||
// a route that has page.params.workspace set; falling back to the
|
||||
// workspace store covers code paths where the editor is rendered
|
||||
@@ -1249,6 +1303,8 @@
|
||||
|
||||
onDestroy(() => {
|
||||
editor?.destroy();
|
||||
themeObserver?.disconnect();
|
||||
themeObserver = null;
|
||||
// onDestroy (unlike onMount) also runs during SSR, where `window` is
|
||||
// undefined — guard with typeof before touching it (TASK-2109).
|
||||
if (typeof window !== 'undefined' && window.visualViewport && visualViewportHandler) {
|
||||
|
||||
@@ -0,0 +1,97 @@
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { mermaidThemeForMode, collectMermaidRerenders } from './mermaidTheme';
|
||||
|
||||
// BUG-3106: initMermaid passed theme:'dark' unconditionally, so every diagram
|
||||
// was drawn on the dark palette in light mode.
|
||||
|
||||
function html(attr: string | null): Element {
|
||||
const el = document.createElement('html');
|
||||
if (attr !== null) el.setAttribute('data-theme', attr);
|
||||
return el;
|
||||
}
|
||||
|
||||
describe('mermaidThemeForMode', () => {
|
||||
// The bug itself: this is the case that was wrong, and it is the one
|
||||
// assertion that fails against the unfixed code.
|
||||
it('light mode asks for the light palette', () => {
|
||||
expect(mermaidThemeForMode(html('light'))).toBe('default');
|
||||
});
|
||||
|
||||
it('explicit dark mode asks for the dark palette', () => {
|
||||
expect(mermaidThemeForMode(html('dark'))).toBe('dark');
|
||||
});
|
||||
|
||||
// The asymmetry worth pinning. +layout only ever SETS data-theme — for a
|
||||
// light preference, or from a saved value — and leaves it OFF for the
|
||||
// default, which is dark. A `=== 'dark'` predicate would read absent as
|
||||
// light and break the common case on a fresh browser, so this is the leg
|
||||
// that discriminates between the correct predicate and the plausible
|
||||
// wrong one.
|
||||
it('treats an ABSENT attribute as dark, not as light', () => {
|
||||
expect(mermaidThemeForMode(html(null))).toBe('dark');
|
||||
});
|
||||
|
||||
// An unrecognised value is the app's default mode, not a third palette.
|
||||
it('treats an unrecognised value as dark', () => {
|
||||
expect(mermaidThemeForMode(html('solarized'))).toBe('dark');
|
||||
expect(mermaidThemeForMode(html(''))).toBe('dark');
|
||||
});
|
||||
|
||||
it('survives being called with no root (SSR)', () => {
|
||||
expect(mermaidThemeForMode(null)).toBe('dark');
|
||||
expect(mermaidThemeForMode(undefined)).toBe('dark');
|
||||
});
|
||||
});
|
||||
|
||||
describe('collectMermaidRerenders', () => {
|
||||
function wrapper(opts: { source: string; error?: boolean }): string {
|
||||
return `
|
||||
<div class="mermaid-wrapper">
|
||||
<pre class="code-block mermaid-source"><code class="language-mermaid">${opts.source}</code></pre>
|
||||
<div class="mermaid-diagram${opts.error ? ' mermaid-error' : ''}"></div>
|
||||
</div>`;
|
||||
}
|
||||
|
||||
function root(inner: string): HTMLElement {
|
||||
const el = document.createElement('div');
|
||||
el.innerHTML = inner;
|
||||
return el;
|
||||
}
|
||||
|
||||
it('returns each renderable diagram with its own source', () => {
|
||||
const r = root(wrapper({ source: 'graph TD; A-->B;' }) + wrapper({ source: 'sequenceDiagram' }));
|
||||
const got = collectMermaidRerenders(r);
|
||||
expect(got.map((g) => g.source)).toEqual(['graph TD; A-->B;', 'sequenceDiagram']);
|
||||
// Each target is the diagram node from its OWN wrapper — a pairing bug
|
||||
// would redraw one diagram's source into another's node.
|
||||
const wrappers = r.querySelectorAll('.mermaid-wrapper');
|
||||
expect(got[0].target).toBe(wrappers[0].querySelector('.mermaid-diagram'));
|
||||
expect(got[1].target).toBe(wrappers[1].querySelector('.mermaid-diagram'));
|
||||
});
|
||||
|
||||
it('skips a diagram showing a syntax error', () => {
|
||||
const r = root(wrapper({ source: 'not a diagram', error: true }));
|
||||
expect(collectMermaidRerenders(r)).toEqual([]);
|
||||
});
|
||||
|
||||
it('skips an empty or blank source', () => {
|
||||
const r = root(wrapper({ source: '' }) + wrapper({ source: ' \n ' }));
|
||||
expect(collectMermaidRerenders(r)).toEqual([]);
|
||||
});
|
||||
|
||||
// The error leg must skip for the RIGHT reason: a wrapper identical except
|
||||
// for the error class is collected, so the empty result above is the class
|
||||
// and not the fixture.
|
||||
it('collects the same wrapper once the error class is gone', () => {
|
||||
const r = root(wrapper({ source: 'graph TD; A-->B;', error: true }));
|
||||
expect(collectMermaidRerenders(r)).toEqual([]);
|
||||
r.querySelector('.mermaid-diagram')!.classList.remove('mermaid-error');
|
||||
expect(collectMermaidRerenders(r)).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('ignores non-mermaid code blocks', () => {
|
||||
const r = root(`
|
||||
<pre class="code-block"><code class="language-go">func main() {}</code></pre>`);
|
||||
expect(collectMermaidRerenders(r)).toEqual([]);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,58 @@
|
||||
// BUG-3106: mermaid's palette follows the app's light/dark mode.
|
||||
//
|
||||
// Extracted from Editor.svelte so the two load-bearing decisions — which
|
||||
// theme the current mode calls for, and which diagrams a mode change has to
|
||||
// redraw — are reachable from a test. Editor.svelte imports both.
|
||||
|
||||
export type MermaidTheme = 'dark' | 'default';
|
||||
|
||||
/**
|
||||
* Which mermaid theme the app's CURRENT mode calls for.
|
||||
*
|
||||
* There is no theme store to read. Four components (+layout's initializer,
|
||||
* TopBar, Sidebar, YouSheet) each toggle `data-theme` on <html> and mirror it
|
||||
* to localStorage['pad-theme']; the ATTRIBUTE is the one thing all four agree
|
||||
* on, so it is what this reads.
|
||||
*
|
||||
* ABSENT MEANS DARK, and that asymmetry is the app's rather than ours:
|
||||
* +layout only ever SETS the attribute — to 'light' for a light preference,
|
||||
* or to the saved value when there is one — and leaves it off entirely for
|
||||
* the default. So the predicate has to be `!== 'light'`. Writing `=== 'dark'`
|
||||
* would send the default mode, which is the common case on a fresh browser,
|
||||
* down the light branch and draw every diagram on the wrong palette.
|
||||
*/
|
||||
export function mermaidThemeForMode(root: Element | null | undefined): MermaidTheme {
|
||||
if (!root) return 'dark';
|
||||
return root.getAttribute('data-theme') === 'light' ? 'default' : 'dark';
|
||||
}
|
||||
|
||||
/**
|
||||
* The diagrams a theme change has to redraw, within one editor.
|
||||
*
|
||||
* Each mermaid code block's NodeView builds the same DOM shape: a
|
||||
* `.mermaid-wrapper` holding the source `code.language-mermaid` and the
|
||||
* `.mermaid-diagram` the SVG lands in. That shape is the only handle on the
|
||||
* live set — ProseMirror creates the NodeViews and nothing enumerates them.
|
||||
*
|
||||
* Two kinds of wrapper are SKIPPED, both deliberately:
|
||||
* - one whose diagram carries `mermaid-error`, because it is showing the
|
||||
* "Invalid Mermaid syntax" message and a re-render would replace that
|
||||
* message with the identical message, flickering an error the user is
|
||||
* already reading;
|
||||
* - one whose source is empty or blank, which has nothing to draw.
|
||||
*/
|
||||
export function collectMermaidRerenders(
|
||||
root: Element,
|
||||
): Array<{ source: string; target: HTMLElement }> {
|
||||
const out: Array<{ source: string; target: HTMLElement }> = [];
|
||||
for (const wrapper of root.querySelectorAll('.mermaid-wrapper')) {
|
||||
const diagram = wrapper.querySelector('.mermaid-diagram');
|
||||
const code = wrapper.querySelector('code.language-mermaid');
|
||||
if (!(diagram instanceof HTMLElement) || !code) continue;
|
||||
if (diagram.classList.contains('mermaid-error')) continue;
|
||||
const source = code.textContent ?? '';
|
||||
if (source.trim() === '') continue;
|
||||
out.push({ source, target: diagram });
|
||||
}
|
||||
return out;
|
||||
}
|
||||
Reference in New Issue
Block a user