diff --git a/application/single_app/config.py b/application/single_app/config.py index 3e0d90142..0eb7affbc 100644 --- a/application/single_app/config.py +++ b/application/single_app/config.py @@ -97,7 +97,7 @@ EXECUTOR_TYPE = 'thread' EXECUTOR_MAX_WORKERS = 30 SESSION_TYPE = 'filesystem' -VERSION = "0.261.036" +VERSION = "0.261.037" IS_DEVELOPMENT = is_development_env_enabled() SESSION_COOKIE_SAMESITE = os.getenv('SESSION_COOKIE_SAMESITE', 'Lax') diff --git a/application/single_app/functions_diagram_operations.py b/application/single_app/functions_diagram_operations.py index 809044a6b..2b7a66094 100644 --- a/application/single_app/functions_diagram_operations.py +++ b/application/single_app/functions_diagram_operations.py @@ -88,11 +88,18 @@ def build_diagram_guidance_message(): Keep the source valid so it renders on the first attempt: - Give every node a quoted label, for example `app["Simple Chat App Service"]`. Unquoted parentheses, braces, angle brackets, colons, `#`, and quotes inside a label break the parser. +- Never use `end`, `graph`, `class`, `style`, `subgraph`, or `click` as a node id: they are reserved words and the diagram will not parse. Write `end_state` or `graph_node` instead. +- Close every `subgraph` with a lowercase `end` on its own line. `End` and `END` are not accepted. - Write one statement per line, and use `%%` for comments. - Use `
` inside a quoted label for a line break; do not use raw newlines. - Do not use `click`, `style` with URLs, or any directive that links or navigates. They are stripped before rendering. - Prefer one clear diagram over several near-duplicates, place it directly after the prose it illustrates, and add a short sentence introducing it. +Keep it readable. A diagram is a picture, not a transcript: +- Keep each node label to a short phrase, roughly a handful of words. Split detail across several connected nodes instead of writing one node with a dozen `
` lines in it, which renders as a tall column of text nobody can take in. +- When the user pastes text or ASCII art to be turned into a diagram, translate the structure and summarise the detail. Do not carry placeholders such as ``, literal `{{}}`, or quoted fragments into labels; describe them in words, or leave them to the prose around the diagram. +- Aim for something that fits on a screen. Beyond roughly twenty nodes, split the answer into more than one diagram, each with its own heading. + A diagram is not always the right answer. When the content is narrative, numeric, or a simple list, prose, a table, or a chart is better. Base every node and edge on the source material or the user's own description, and never invent components, systems, or relationships to fill out a picture. Use a diagram, not a generated image, for structural content such as flows, architectures, sequences, and relationships: Mermaid output stays selectable, accessible, and editable. Reserve image generation for illustrative or pictorial visuals. Use inline chart blocks, not Mermaid, when the answer is a plot of numeric or categorical data.""" diff --git a/application/single_app/functions_message_visual_styles.py b/application/single_app/functions_message_visual_styles.py index 5f99ca8b0..f094dc654 100644 --- a/application/single_app/functions_message_visual_styles.py +++ b/application/single_app/functions_message_visual_styles.py @@ -17,8 +17,14 @@ mermaid's theme configuration in a browser, so colours are reduced to `#rrggbb` and nothing else is stored. Sizes are capped so a message document cannot be grown without bound by repeated requests. + +An entry also carries the height someone dragged the block to. That is stored and cleared +independently of the colours, because the two are separate choices: resetting a diagram's +colours should not silently snap it back to its automatic height, and resizing a diagram +should not stop it following the reader's default palette. """ +import math import re # Fence languages a style may be saved against. Matches VISUAL_STYLE_KINDS in @@ -44,6 +50,15 @@ # Total stored entries across every kind, which bounds the size of the stored map. MAX_STORED_ENTRIES = 100 +# Stored block height in pixels. Matches MIN_STAGE_HEIGHT and MAX_STAGE_HEIGHT in +# application/v2_ui/src/components/chat/DiagramStage.tsx. +MIN_BLOCK_HEIGHT = 140 +MAX_BLOCK_HEIGHT = 2000 + +# Sentinel meaning "the caller said nothing about the height", which is different from the +# caller asking for the stored height to be removed. +UNSET = object() + HEX_COLOR_PATTERN = re.compile(r'^#[0-9a-fA-F]{6}$') # Long enough for the 32-bit hex fingerprint the client sends, with room to spare. @@ -94,6 +109,26 @@ def validate_source_hash(value): return candidate +def validate_block_height(value): + """Return a storable block height in pixels, or None to clear a stored one. + + Clamped rather than rejected when out of range. The value comes from a drag, so a request + a few pixels past the limit is a reader holding the mouse down, not a client misbehaving, + and refusing it would lose a change they clearly meant to make. + + Non-finite values are refused rather than clamped. ``json.loads`` accepts the bare + ``Infinity`` and ``NaN`` tokens, and ``round(float('inf'))`` raises ``OverflowError``, which + would escape the caller's ``VisualStyleError`` handling and turn a bad request into a 500. + """ + if value is None: + return None + if isinstance(value, bool) or not isinstance(value, (int, float)): + raise VisualStyleError('Height must be a number') + if not math.isfinite(value): + raise VisualStyleError('Height must be a finite number') + return int(min(MAX_BLOCK_HEIGHT, max(MIN_BLOCK_HEIGHT, round(value)))) + + def sanitize_visual_style(value): """Return a storable style dict, rejecting anything that is not one. @@ -160,30 +195,63 @@ def count_entries(styles): return sum(len(entries) for entries in styles.values()) -def apply_visual_style(message_doc, block_kind, block_index, style, source_hash=''): - """Store, replace or remove one block's colours, returning the resulting map. +def apply_visual_style( + message_doc, + block_kind, + block_index, + style, + source_hash='', + height=UNSET, +): + """Store, replace or remove one block's colours and height, returning the resulting map. - ``style`` of None removes the entry, which is different from storing a style that happens + ``style`` of None removes the colours, which is different from storing a style that happens to equal the reader's current default: the default can change later, and a removed entry should follow it. + + ``height`` left at ``UNSET`` keeps whatever is stored, so a colour change does not disturb a + size someone chose. ``None`` clears it. The entry itself only disappears once it holds + neither colours nor a height. """ kind = validate_block_kind(block_kind) index = validate_block_index(block_index) fingerprint = validate_source_hash(source_hash) + resolved_height = UNSET if height is UNSET else validate_block_height(height) styles = read_visual_styles(message_doc) entries = dict(styles.get(kind) or {}) + existing = entries.get(str(index)) + existing = existing if isinstance(existing, dict) else {} + + # A stored entry whose fingerprint no longer matches describes different content, and the + # client already ignores it. Carrying its height forward would resurrect it and stamp it + # with the new fingerprint, making a size chosen for a block that no longer exists at this + # position authoritative for the one that does. + existing_hash = existing.get('source_hash') + if isinstance(existing_hash, str) and existing_hash and fingerprint and existing_hash != fingerprint: + existing = {} if style is None: - entries.pop(str(index), None) + entry = {} else: - sanitized = sanitize_visual_style(style) + entry = sanitize_visual_style(style) + + if resolved_height is UNSET: + kept_height = existing.get('height') + if isinstance(kept_height, int) and not isinstance(kept_height, bool): + entry['height'] = kept_height + elif resolved_height is not None: + entry['height'] = resolved_height + + if entry: if fingerprint: - sanitized['source_hash'] = fingerprint + entry['source_hash'] = fingerprint is_new = str(index) not in entries if is_new and count_entries(styles) >= MAX_STORED_ENTRIES: raise VisualStyleError('Too many styled blocks in this message') - entries[str(index)] = sanitized + entries[str(index)] = entry + else: + entries.pop(str(index), None) if entries: styles[kind] = entries diff --git a/application/single_app/route_backend_chats.py b/application/single_app/route_backend_chats.py index 9d667ee58..1241c78de 100644 --- a/application/single_app/route_backend_chats.py +++ b/application/single_app/route_backend_chats.py @@ -235,6 +235,7 @@ resolve_mask_display_name, ) from functions_message_visual_styles import ( + UNSET as VISUAL_STYLE_HEIGHT_UNSET, VisualStyleError, apply_visual_style, ) @@ -24902,13 +24903,16 @@ def mask_message_api(message_id): @login_required @user_required def set_message_visual_style_api(message_id): - """Save or clear the colours chosen for one diagram or chart inside a message. + """Save or clear the colours and height chosen for one diagram or chart in a message. A reply can contain several styleable blocks, so the request identifies one of them by its position among blocks of the same kind. Recolouring one block therefore leaves the others untouched, which is the whole point of storing this per block rather than per message. + ``height`` is optional and independent of ``style``: omitting it keeps whatever size the + block was left at, so changing colours never resets a diagram someone resized. + Unlike the classic client's chart colour editor, nothing here rewrites the message content: the payload the model produced stays exactly as it was written, and the colours live beside it in metadata. @@ -24956,6 +24960,9 @@ def set_message_visual_style_api(message_id): data.get('block_index'), data.get('style'), data.get('source_hash') or '', + # A body that never mentions the height leaves the stored one alone; one + # that sends null is asking for it to be cleared. + data.get('height') if 'height' in data else VISUAL_STYLE_HEIGHT_UNSET, ) except VisualStyleError as ex: debug_print(f'[VISUAL_STYLE] Invalid request: {ex}') diff --git a/application/v2_ui/src/components/chat/DiagramStage.tsx b/application/v2_ui/src/components/chat/DiagramStage.tsx new file mode 100644 index 000000000..de8d863ba --- /dev/null +++ b/application/v2_ui/src/components/chat/DiagramStage.tsx @@ -0,0 +1,308 @@ +// DiagramStage.tsx +// The scrolling, resizable area a rendered diagram is drawn in. +// +// Split out of MermaidDiagram so that the sizing behaviour — which is most of what makes a +// diagram readable — is separable from rendering it. This file deliberately contains no HTML +// sink: the markup it shows is passed in already sanitized, and every place diagram markup is +// written into the DOM stays in MermaidDiagram.tsx, so the reviewed sanitizer boundary remains +// a single file. +// +// Two numbers drive everything here: +// +// - the diagram's natural size, read off the SVG mermaid emitted. Mermaid renders with +// `useMaxWidth: true`, which produces `width="100%"`, no height attribute and +// `style="max-width: Npx"`. A percentage width contributes nothing to intrinsic sizing, so +// a diagram in the shrink-to-fit assistant bubble collapsed the bubble to the width of its +// own toolbar and then scaled itself down to match. Reading N back and applying it as a +// definite width is what stops that. +// +// - the stage height, which is capped. A flowchart with a few hundred edges renders tens of +// thousands of pixels tall; left in the message list, the browser re-rasterizes it on every +// scroll frame and the thread becomes unusable. + +import { useCallback, useEffect, useRef, useState } from 'react'; + +/** Smallest stage a reader can drag to. Below this the diagram is not worth showing. */ +export const MIN_STAGE_HEIGHT = 140; + +/** Largest stage a reader can drag to, and the ceiling on a persisted height. */ +export const MAX_STAGE_HEIGHT = 2000; + +/** + * Tallest a diagram is drawn at before its stage starts scrolling. + * + * Roughly two thirds of a laptop viewport: tall enough that most diagrams are shown whole, + * short enough that a very long one does not push the rest of the reply off the screen or + * leave a huge SVG in the scroll container. + */ +export const DEFAULT_MAX_STAGE_HEIGHT = 520; + +/** Narrowest the panel goes, so the toolbar never wraps into a column. */ +export const MIN_FIGURE_WIDTH = 320; + +/** + * The stage's own padding, top and bottom. + * + * Kept as a number because the automatic height has to account for it: the height is set on the + * padding box, so a stage sized to the diagram alone is short by exactly this much and shows a + * scrollbar on a diagram that actually fits. + */ +const STAGE_PADDING = 24; + +/** Zoom bounds, as a multiple of the scale that fits the diagram to the stage width. */ +export const MIN_ZOOM = 0.4; +export const MAX_ZOOM = 4; + +/** Multiplier per press of the zoom buttons. */ +export const ZOOM_STEP = 1.25; + +/** How much one arrow-key press moves the resize handle. */ +const RESIZE_KEY_STEP = 40; + +export interface DiagramSize { + width: number; + height: number; +} + +export function clampStageHeight(value: number): number { + return Math.min(MAX_STAGE_HEIGHT, Math.max(MIN_STAGE_HEIGHT, Math.round(value))); +} + +export function clampZoom(value: number): number { + return Math.min(MAX_ZOOM, Math.max(MIN_ZOOM, value)); +} + +/** + * Read a rendered diagram's natural size out of the SVG markup. + * + * `max-width` is what mermaid writes when `useMaxWidth` is on and is the authoritative natural + * width; the viewBox is the fallback and the only source of the height, because mermaid writes + * no height attribute in that mode. Returns null rather than a guess when neither is present, + * so the caller can fall back to letting the browser size the diagram as it did before. + */ +export function readDiagramSize(svg: string): DiagramSize | null { + const viewBox = /viewBox="([^"]*)"/.exec(svg); + const parts = viewBox + ? viewBox[1].trim().split(/[\s,]+/).map(Number) + : []; + const boxWidth = parts.length === 4 && Number.isFinite(parts[2]) ? parts[2] : 0; + const boxHeight = parts.length === 4 && Number.isFinite(parts[3]) ? parts[3] : 0; + + const maxWidth = /max-width:\s*([0-9.]+)px/.exec(svg); + const declaredWidth = maxWidth ? Number(maxWidth[1]) : 0; + + const width = Number.isFinite(declaredWidth) && declaredWidth > 0 ? declaredWidth : boxWidth; + if (!(width > 0) || !(boxHeight > 0)) { + return null; + } + + // The height that goes with `width`, since the two can differ when mermaid's declared + // max-width has been rounded away from the viewBox. + const height = boxWidth > 0 ? (boxHeight * width) / boxWidth : boxHeight; + return { width: Math.round(width), height: Math.round(height) }; +} + +/** + * The stage height to use when nobody has chosen one. + * + * The diagram is fitted to the panel width first, because that is how it will actually be + * drawn, and the resulting height is then capped. A wide, short diagram therefore gets a short + * stage rather than an empty one, and a tall diagram gets a scrolling stage rather than a + * thousand-pixel block in the thread. + * + * `panelWidth` is the stage's content width, so the padding is added back afterwards. + */ +export function defaultStageHeight(size: DiagramSize | null, panelWidth: number): number { + if (!size || panelWidth <= 0) { + return MIN_STAGE_HEIGHT; + } + const fitted = size.height * Math.min(1, panelWidth / size.width); + return clampStageHeight(Math.min(fitted + STAGE_PADDING, DEFAULT_MAX_STAGE_HEIGHT)); +} + +/** + * A grab bar for resizing the stage. + * + * A separate control rather than CSS `resize`, which cannot be operated from the keyboard and + * offers no way back to the automatic height. Exposed as a slider because that is what it is: + * a single value with a range, a current position and a meaningful reset. + */ +function ResizeHandle({ + height, + onResize, + onReset, +}: { + height: number; + onResize: (next: number) => void; + onReset: () => void; +}) { + const dragRef = useRef<{ startY: number; startHeight: number } | null>(null); + + const onPointerDown = (event: React.PointerEvent) => { + event.preventDefault(); + dragRef.current = { startY: event.clientY, startHeight: height }; + event.currentTarget.setPointerCapture(event.pointerId); + }; + + const onPointerMove = (event: React.PointerEvent) => { + const drag = dragRef.current; + if (!drag) { + return; + } + onResize(clampStageHeight(drag.startHeight + (event.clientY - drag.startY))); + }; + + const endDrag = (event: React.PointerEvent) => { + if (!dragRef.current) { + return; + } + dragRef.current = null; + if (event.currentTarget.hasPointerCapture(event.pointerId)) { + event.currentTarget.releasePointerCapture(event.pointerId); + } + }; + + return ( +
{ + if (event.key === 'ArrowDown' || event.key === 'ArrowRight') { + event.preventDefault(); + onResize(clampStageHeight(height + RESIZE_KEY_STEP)); + } else if (event.key === 'ArrowUp' || event.key === 'ArrowLeft') { + event.preventDefault(); + onResize(clampStageHeight(height - RESIZE_KEY_STEP)); + } else if (event.key === 'Home') { + event.preventDefault(); + onReset(); + } + }} + className="group/handle flex cursor-ns-resize touch-none items-center justify-center py-1 outline-none" + > +
+ ); +} + +/** + * The area a diagram is drawn in, with its own scrolling and a handle to resize it. + * + * `children` is the already-rendered diagram. This component owns only how much room it gets + * and how far it is scaled; it never touches the markup. + */ +export function DiagramStage({ + size, + height, + zoom, + onResize, + onResetHeight, + onPanelWidth, + background, + children, +}: { + size: DiagramSize | null; + height: number; + /** Multiplier applied on top of fitting the diagram to the stage width. */ + zoom: number; + onResize: (next: number) => void; + onResetHeight: () => void; + /** Reports the stage's laid-out width, which the fit scale is computed from. */ + onPanelWidth: (width: number) => void; + background?: string; + children: React.ReactNode; +}) { + const stageRef = useRef(null); + const [panelWidth, setPanelWidth] = useState(0); + + const report = useCallback( + (width: number) => { + setPanelWidth(width); + onPanelWidth(width); + }, + [onPanelWidth], + ); + + // The stage width is what the fit scale divides by, and it changes when the window is + // resized, the sidebar is toggled or the reading-width preference is changed. + useEffect(() => { + const element = stageRef.current; + if (!element || typeof ResizeObserver === 'undefined') { + return; + } + report(element.clientWidth); + const observer = new ResizeObserver((entries) => { + const width = entries[0]?.contentRect.width ?? element.clientWidth; + report(width); + }); + observer.observe(element); + return () => observer.disconnect(); + }, [report]); + + const fitScale = size && panelWidth > 0 ? Math.min(1, panelWidth / size.width) : 1; + const scale = fitScale * zoom; + + return ( + <> +
+
+
+ {children} +
+
+
+ + + + ); +} diff --git a/application/v2_ui/src/components/chat/MermaidDiagram.tsx b/application/v2_ui/src/components/chat/MermaidDiagram.tsx index c219a82d7..e29f0ea1d 100644 --- a/application/v2_ui/src/components/chat/MermaidDiagram.tsx +++ b/application/v2_ui/src/components/chat/MermaidDiagram.tsx @@ -8,9 +8,23 @@ // The library is loaded from the vendored copy on first use rather than bundled. It is 3.4 MB // — larger than the rest of the application put together — and most conversations never show // a diagram. +// +// Sizing lives in DiagramStage.tsx. The expanded viewer lives here rather than in its own file +// on purpose: it writes diagram markup to the DOM, and keeping every such sink in one reviewed +// file is what test_v2_rich_rendering.py's sanitizer boundary check is protecting. -import { useEffect, useMemo, useRef, useState } from 'react'; -import { Download, TriangleAlert } from 'lucide-react'; +import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import { + ChevronDown, + Copy, + Download, + Maximize2, + Minus, + Plus, + Scan, + TriangleAlert, + X, +} from 'lucide-react'; import { useUiStore } from '../../stores/uiStore'; import type { DomPurifyStatic, MermaidStatic } from '../../lib/vendor'; import { VENDOR_PATHS, loadDomPurify, loadVendorScript } from '../../lib/vendorAssets'; @@ -23,9 +37,26 @@ import { visualStyleSignature, type VisualStyle, } from '../../lib/visualPalettes'; +import { + describeMermaidError, + isRepairWorthTrying, + repairMermaidSource, +} from '../../lib/mermaidSource'; import { downloadDataUri, fileNameStem, svgElementToPngDataUri } from '../../lib/svgRaster'; import { registerExportDiagram } from '../../lib/exportVisuals'; import { VisualStyleMenu } from './VisualStyleMenu'; +import { + clampZoom, + defaultStageHeight, + DiagramStage, + MAX_ZOOM, + MIN_FIGURE_WIDTH, + MIN_ZOOM, + readDiagramSize, + ZOOM_STEP, + type DiagramSize, +} from './DiagramStage'; +import { GlassPanel } from '../ui/primitives'; interface MermaidRuntime { mermaid: MermaidStatic; @@ -37,6 +68,44 @@ let runtimeLoad: Promise | null = null; let configuredSignature: string | null = null; let idCounter = 0; +/** + * How long one diagram is given before it is treated as failed. + * + * Matches MERMAID_RENDER_TIMEOUT_MS in static/js/chat/chat-mermaid-runtime.js. Without it a + * render that never settles leaves "Rendering diagram…" on screen for the life of the page, + * which is indistinguishable from a diagram that is merely slow. + */ +const RENDER_TIMEOUT_MS = 10000; + +/** + * Longest diagram source that is attempted at all. + * + * Matches INLINE_DIAGRAM_MAX_SOURCE_LENGTH in static/js/chat/chat-inline-diagrams.js. Mermaid + * has its own `maxTextSize`, but it reports the refusal as a render failure; checking first + * lets the reader be told the diagram is too large rather than that it is broken. + */ +const MAX_SOURCE_LENGTH = 30000; + +/** + * Ceilings handed to mermaid, set explicitly rather than left to its defaults. + * + * `maxEdges` matters: a diagram at mermaid's default limit of 500 edges renders roughly fifty + * thousand pixels tall, so the limit is a rendering safeguard as much as a parsing one and is + * worth stating where it can be seen. + */ +const MERMAID_MAX_TEXT_SIZE = 50000; +const MERMAID_MAX_EDGES = 500; + +/** + * How wide a label is allowed to get before mermaid wraps it. + * + * Mermaid's default is 200px, which turns the long labels models write into narrow columns of + * text: the same diagram measures 273 x 955 at the default and 497 x 867 at this value, so the + * default is actively making diagrams taller and harder to read. Diagrams that break their own + * labels with `
`, which is what the diagram guidance asks for, are unaffected. + */ +const MERMAID_WRAPPING_WIDTH = 500; + /** * Serialises rendering. * @@ -108,10 +177,18 @@ function configure( // rasterizable: a label disappears when an SVG is painted onto a // canvas, which would produce a PNG with no text in it. htmlLabels: false, - flowchart: { htmlLabels: false, useMaxWidth: true }, + flowchart: { + htmlLabels: false, + useMaxWidth: true, + wrappingWidth: MERMAID_WRAPPING_WIDTH, + }, class: { htmlLabels: false, useMaxWidth: true }, sequence: { useMaxWidth: true }, gantt: { useMaxWidth: true }, + // Stated rather than inherited, so the ceilings a diagram is measured against are + // visible next to the code that has to explain hitting them. + maxTextSize: MERMAID_MAX_TEXT_SIZE, + maxEdges: MERMAID_MAX_EDGES, // Mermaid otherwise writes its own error diagram straight into the page, outside // React's control. Failures are handled below instead. suppressErrorRendering: true, @@ -147,6 +224,26 @@ function cacheSvg(key: string, svg: string) { svgCache.set(key, svg); } +/** Reject once a render has had long enough, without cancelling the render itself. */ +function withTimeout(work: Promise, timeoutMs: number): Promise { + return new Promise((resolve, reject) => { + const timer = setTimeout( + () => reject(new Error('Timed out rendering the diagram.')), + timeoutMs, + ); + work.then( + (value) => { + clearTimeout(timer); + resolve(value); + }, + (error) => { + clearTimeout(timer); + reject(error); + }, + ); + }); +} + async function renderDiagram( source: string, theme: string, @@ -161,6 +258,10 @@ async function renderDiagram( return cached; } + if (source.length > MAX_SOURCE_LENGTH) { + throw new Error('The diagram source is too large to draw.'); + } + const { mermaid, purify } = await loadMermaidRuntime(); const run = renderQueue.then(async () => { @@ -171,8 +272,27 @@ async function renderDiagram( configure(mermaid, theme, style, background, configKey); - idCounter += 1; - const { svg } = await mermaid.render(`simplechat-mermaid-${idCounter}`, source); + const draw = async (text: string) => { + idCounter += 1; + const { svg } = await withTimeout( + Promise.resolve(mermaid.render(`simplechat-mermaid-${idCounter}`, text)), + RENDER_TIMEOUT_MS, + ); + return svg; + }; + + let svg: string; + try { + svg = await draw(source); + } catch (error) { + // Second attempt only. Repairing source mermaid has already accepted would risk + // changing a diagram that is drawing correctly, so the original is always tried + // first and the rewrite is a last resort before showing the reader the source. + if (!isRepairWorthTrying(source)) { + throw error; + } + svg = await draw(repairMermaidSource(source)); + } // Sanitizer boundary. Mermaid's 'strict' level already sanitizes internally; this // is the independent second pass required before model-derived markup is written @@ -191,16 +311,75 @@ async function renderDiagram( type DiagramState = | { status: 'pending' } | { status: 'ready'; svg: string } - | { status: 'error' }; + | { status: 'error'; reason: string }; -/** The diagram source, shown when it cannot be rendered. */ +/** + * The diagram source, shown when it cannot be rendered. + * + * The reason mermaid gave is shown rather than swallowed. Before this the only signal a reader + * or an administrator had was the words "Diagram could not be rendered", which is not enough to + * tell a malformed diagram from a library that failed to load. + */ function DiagramSource({ source, reason }: { source: string; reason: string }) { + const [detailsOpen, setDetailsOpen] = useState(false); + const [copied, setCopied] = useState(false); + const copyTimerRef = useRef | null>(null); + + useEffect( + () => () => { + if (copyTimerRef.current !== null) { + clearTimeout(copyTimerRef.current); + } + }, + [], + ); + + const copySource = async () => { + try { + await navigator.clipboard.writeText(source); + setCopied(true); + if (copyTimerRef.current !== null) { + clearTimeout(copyTimerRef.current); + } + copyTimerRef.current = setTimeout(() => setCopied(false), 2000); + } catch { + setCopied(false); + } + }; + return (
-
+
- {reason} + Diagram could not be rendered + +
+ + {detailsOpen && ( +

+ {reason} +

+ )} + 
                 {source}
@@ -208,21 +387,219 @@ function DiagramSource({ source, reason }: { source: string; reason: string }) { ); } -/** The first line of a diagram, used to name its downloaded file. */ +/** The first line of a diagram, used to name its downloaded file and title its viewer. */ function diagramName(source: string): string { const firstLine = source.trim().split('\n', 1)[0] ?? ''; return fileNameStem(firstLine, 'diagram'); } +/** Zoom in, zoom out and fit, shared by the inline panel and the expanded viewer. */ +function ZoomControls({ + zoom, + onZoom, + onReset, + compact = false, +}: { + zoom: number; + onZoom: (next: number) => void; + onReset: () => void; + compact?: boolean; +}) { + const buttonClass = compact + ? 'shrink-0 rounded-lg p-1.5 text-text-3 transition-colors hover:bg-surface-2 hover:text-text-1 disabled:cursor-not-allowed disabled:opacity-40' + : 'inline-flex items-center rounded-md px-1.5 py-1 text-text-3 transition-colors hover:bg-surface-2 hover:text-text-1 disabled:cursor-not-allowed disabled:opacity-40'; + + return ( + <> + + + + + ); +} + +/** + * Full-screen view of one diagram. + * + * Follows the conventions ImageLightbox established: a click-to-close backdrop, Escape to + * dismiss, focus moved in on open and handed back on close, and no focus-trap utility, because + * none of the other dialogs in this interface use one. + * + * The diagram is drawn from the same sanitized markup the inline panel is showing, so nothing + * is re-rendered and the two cannot disagree. + */ +function DiagramLightbox({ + svg, + size, + title, + background, + onDownload, + onClose, +}: { + svg: string; + size: DiagramSize | null; + title: string; + background?: string; + onDownload: (element: SVGElement | null) => void; + onClose: () => void; +}) { + const [zoom, setZoom] = useState(1); + const [fitWidth, setFitWidth] = useState(0); + const closeRef = useRef(null); + const viewportRef = useRef(null); + const contentRef = useRef(null); + + useEffect(() => { + const onKeyDown = (event: KeyboardEvent) => { + if (event.key === 'Escape') { + onClose(); + } + }; + document.addEventListener('keydown', onKeyDown); + return () => document.removeEventListener('keydown', onKeyDown); + }, [onClose]); + + // Opening a dialog should move the keyboard focus into it, and closing should hand it back + // to whatever had it before, which is the button that opened it. + useEffect(() => { + const previous = document.activeElement as HTMLElement | null; + closeRef.current?.focus(); + return () => previous?.focus?.(); + }, []); + + useEffect(() => { + const element = viewportRef.current; + if (!element || typeof ResizeObserver === 'undefined') { + return; + } + setFitWidth(element.clientWidth); + const observer = new ResizeObserver((entries) => { + setFitWidth(entries[0]?.contentRect.width ?? element.clientWidth); + }); + observer.observe(element); + return () => observer.disconnect(); + }, []); + + const scale = size && fitWidth > 0 ? Math.min(1, fitWidth / size.width) * zoom : zoom; + + return ( +
+ ); diff --git a/application/v2_ui/src/lib/blockVisualStyle.ts b/application/v2_ui/src/lib/blockVisualStyle.ts index f2122c203..ff9def3e2 100644 --- a/application/v2_ui/src/lib/blockVisualStyle.ts +++ b/application/v2_ui/src/lib/blockVisualStyle.ts @@ -1,10 +1,15 @@ // blockVisualStyle.ts -// Resolves the colours one diagram or chart should use, and saves a change to them. +// Resolves the colours and the size one diagram or chart should use, and saves a change to +// either. // -// Three sources are consulted, in increasing precedence: the built-in default, the reader's own -// default from their settings document, and an override saved against this specific block of -// this specific message. Because the override wins outright, recolouring one chart leaves every -// other chart in the conversation exactly as it was. +// Three sources are consulted for colours, in increasing precedence: the built-in default, the +// reader's own default from their settings document, and an override saved against this +// specific block of this specific message. Because the override wins outright, recolouring one +// chart leaves every other chart in the conversation exactly as it was. +// +// The size is simpler: a block either has a height someone dragged it to or it does not, and +// the two are stored in the same entry but changed independently. Resetting colours must not +// resize a block, and resizing a block must not stop it following the reader's palette. // // A block has no identity of its own, so an override is addressed by the block's position among // blocks of the same kind in the message, together with a fingerprint of its source. If the @@ -38,13 +43,13 @@ export const VISUAL_STYLE_SETTING_KEYS: Record = { */ const SAVE_DEBOUNCE_MS = 400; -/** The stored override for one block, or null when there is none that still applies. */ -function readStoredOverride( +/** The stored entry for one block, or null when there is none that still applies. */ +function readStoredEntry( metadata: unknown, kind: VisualStyleKind, blockIndex: number, sourceHash: string, -): VisualStyle | null { +): Record | null { if (!metadata || typeof metadata !== 'object') { return null; } @@ -71,9 +76,45 @@ function readStoredOverride( return null; } + return entry as Record; +} + +/** + * The stored colour override for one block, or null when there is none. + * + * An entry that carries only a height is not a colour override. Treating it as one would stop + * a diagram someone merely resized from following the reader's default palette, which is a + * change they never asked for. The presence of `palette` is what distinguishes the two, + * because the server writes the colour fields together or not at all. + */ +function readStoredOverride( + metadata: unknown, + kind: VisualStyleKind, + blockIndex: number, + sourceHash: string, +): VisualStyle | null { + const entry = readStoredEntry(metadata, kind, blockIndex, sourceHash); + if (!entry || typeof entry.palette !== 'string') { + return null; + } return sanitizeVisualStyle(entry); } +/** The stored stage height for one block, or null when it has none. */ +function readStoredHeight( + metadata: unknown, + kind: VisualStyleKind, + blockIndex: number, + sourceHash: string, +): number | null { + const entry = readStoredEntry(metadata, kind, blockIndex, sourceHash); + const height = entry?.height; + if (typeof height !== 'number' || !Number.isFinite(height) || height <= 0) { + return null; + } + return Math.round(height); +} + export interface BlockVisualStyle { /** The colours to render with. */ style: VisualStyle; @@ -81,6 +122,12 @@ export interface BlockVisualStyle { setStyle: (next: VisualStyle) => void; /** Drop the block's own colours so it follows the reader's default again. */ reset: () => void; + /** The height the block was left at, or null to size it automatically. */ + height: number | null; + /** Resize the block, immediately on screen and shortly afterwards on the server. */ + setHeight: (next: number) => void; + /** Drop the chosen height so the block is sized automatically again. */ + resetHeight: () => void; /** * True when the choice will be kept. * @@ -92,6 +139,19 @@ export interface BlockVisualStyle { error: string | null; } +/** + * One queued change. + * + * `undefined` on either field means "this change says nothing about that", which is how a + * resize leaves the colours alone and a recolour leaves the size alone. Queued changes merge + * rather than replace, so a recolour immediately followed by a drag still writes both. + */ +interface PendingChange { + style?: VisualStyle | null; + height?: number | null; + conversationId: string; +} + export function useBlockVisualStyle( kind: VisualStyleKind, source: string, @@ -125,44 +185,68 @@ export function useBlockVisualStyle( : null, [addressable, storedMetadata, kind, blockIndex, sourceHash], ); + const storedHeight = useMemo( + () => + addressable + ? readStoredHeight(storedMetadata, kind, blockIndex as number, sourceHash) + : null, + [addressable, storedMetadata, kind, blockIndex, sourceHash], + ); /** The reader's unsaved change, which takes precedence until the write settles. */ const [draft, setDraft] = useState<{ value: VisualStyle | null } | null>(null); + const [heightDraft, setHeightDraft] = useState<{ value: number | null } | null>(null); const [error, setError] = useState(null); const applyVisualStyle = useChatStore((state) => state.applyVisualStyle); const timerRef = useRef | null>(null); /** The change waiting to be written, with the conversation it was made in. */ - const pendingRef = useRef<{ value: VisualStyle | null; conversationId: string } | null>( - null, - ); + const pendingRef = useRef(null); + + const style = useMemo(() => { + const override = draft ? draft.value : storedOverride; + return resolveVisualStyle(userDefault, override) ?? DEFAULT_VISUAL_STYLE; + }, [draft, storedOverride, userDefault]); - /** Write a value now, without waiting for the debounce. */ + const height = heightDraft ? heightDraft.value : storedHeight; + + // Read when a resize is scheduled, so the write carries the colours the block actually has + // rather than re-deriving them: sending the resolved style would pin a block that is only + // following the reader's default. + const effectiveOverrideRef = useRef(null); + effectiveOverrideRef.current = draft ? draft.value : storedOverride; + + /** Write a change now, without waiting for the debounce. */ const write = useCallback( - (value: VisualStyle | null, conversationId: string) => { + (change: PendingChange) => { if (!messageId || typeof blockIndex !== 'number') { return; } + const styleForWrite = + change.style === undefined ? effectiveOverrideRef.current : change.style; + void applyVisualStyle( messageId, - conversationId, + change.conversationId, kind, blockIndex, sourceHash, - value, + styleForWrite, + change.height, ).then((saved) => { - // Either way the draft is dropped: on success the store now holds the stored - // value, and on failure the block should show what is actually saved rather + // Either way the drafts are dropped: on success the store now holds the stored + // values, and on failure the block should show what is actually saved rather // than a change that never landed. setDraft(null); - setError(saved ? null : 'Those colours could not be saved.'); + setHeightDraft(null); + setError(saved ? null : 'That change could not be saved.'); }); }, [applyVisualStyle, blockIndex, kind, messageId, sourceHash], ); const schedule = useCallback( - (value: VisualStyle | null) => { + (change: Omit) => { if (timerRef.current !== null) { clearTimeout(timerRef.current); timerRef.current = null; @@ -178,11 +262,18 @@ export function useBlockVisualStyle( return; } - pendingRef.current = { value, conversationId }; + const merged: PendingChange = { + ...(pendingRef.current ?? {}), + ...(change.style === undefined ? {} : { style: change.style }), + ...(change.height === undefined ? {} : { height: change.height }), + conversationId, + }; + + pendingRef.current = merged; timerRef.current = setTimeout(() => { timerRef.current = null; pendingRef.current = null; - write(value, conversationId); + write(merged); }, SAVE_DEBOUNCE_MS); }, [addressable, write], @@ -199,9 +290,9 @@ export function useBlockVisualStyle( timerRef.current = null; } if (pendingRef.current) { - const { value, conversationId } = pendingRef.current; + const change = pendingRef.current; pendingRef.current = null; - writeRef.current(value, conversationId); + writeRef.current(change); } }, [], @@ -211,7 +302,7 @@ export function useBlockVisualStyle( (next: VisualStyle) => { setError(null); setDraft({ value: next }); - schedule(next); + schedule({ style: next }); }, [schedule], ); @@ -219,13 +310,32 @@ export function useBlockVisualStyle( const reset = useCallback(() => { setError(null); setDraft({ value: null }); - schedule(null); + schedule({ style: null }); }, [schedule]); - const style = useMemo(() => { - const override = draft ? draft.value : storedOverride; - return resolveVisualStyle(userDefault, override) ?? DEFAULT_VISUAL_STYLE; - }, [draft, storedOverride, userDefault]); + const setHeight = useCallback( + (next: number) => { + setError(null); + setHeightDraft({ value: Math.round(next) }); + schedule({ height: Math.round(next) }); + }, + [schedule], + ); + + const resetHeight = useCallback(() => { + setError(null); + setHeightDraft({ value: null }); + schedule({ height: null }); + }, [schedule]); - return { style, setStyle, reset, canPersist: addressable, error }; + return { + style, + setStyle, + reset, + height, + setHeight, + resetHeight, + canPersist: addressable, + error, + }; } diff --git a/application/v2_ui/src/lib/endpoints.ts b/application/v2_ui/src/lib/endpoints.ts index de90aeb73..ab2470d68 100644 --- a/application/v2_ui/src/lib/endpoints.ts +++ b/application/v2_ui/src/lib/endpoints.ts @@ -241,12 +241,14 @@ export const maskMessage = ( /* Message visual styles */ /* -------------------------------------------------------------------------- */ -/** Colours saved against one diagram or chart inside a message. */ +/** Colours and size saved against one diagram or chart inside a message. */ export interface VisualStyleEntry { palette?: string; background?: string; colors?: Record; source_hash?: string; + /** Stage height in pixels, set by dragging the block's resize handle. */ + height?: number; } /** Every saved entry for a message, keyed by fence language then by block index. */ @@ -260,11 +262,15 @@ export interface VisualStyleResponse { } /** - * Save, or clear, the colours for one block of one message. + * Save, or clear, the colours and size for one block of one message. * - * A null `style` removes the entry so the block follows the reader's own default again, which - * is a different outcome from saving a style that happens to equal that default: the default - * can change later. + * A null `style` removes the colours so the block follows the reader's own default again, + * which is a different outcome from saving a style that happens to equal that default: the + * default can change later. + * + * `height` is deliberately optional rather than nullable-by-default. Omitting the key leaves + * whatever size is stored alone, so changing colours never resets a diagram someone resized; + * sending null is what clears it. * * `conversation_id` lets the server read the message by partition key rather than running a * cross-partition query, exactly as the mask endpoint does. @@ -277,6 +283,7 @@ export const setMessageVisualStyle = ( block_index: number; source_hash: string; style: { palette: string; background: string; colors: Record } | null; + height?: number | null; }, ) => api.post( diff --git a/application/v2_ui/src/lib/mermaidSource.ts b/application/v2_ui/src/lib/mermaidSource.ts new file mode 100644 index 000000000..c5c55abef --- /dev/null +++ b/application/v2_ui/src/lib/mermaidSource.ts @@ -0,0 +1,389 @@ +// mermaidSource.ts +// Repairs the mistakes models actually make when writing Mermaid, so a diagram that would +// otherwise be shown as a wall of source renders instead. +// +// Every rule here corresponds to a failure reproduced against the vendored mermaid 11.17.2 +// bundle in Chromium, configured exactly as MermaidDiagram.tsx configures it. Nothing is +// speculative: a rule that could not be made to fail was not written. +// +// This runs only on a SECOND attempt, after mermaid has already rejected the source. A diagram +// that renders today is handed to mermaid untouched and is never rewritten, so this cannot +// regress anything that currently works. That ordering matters more than the rules themselves: +// repairing model output is inherently lossy, and the correct source is always preferred. + +/** + * Words the flowchart grammar reserves, which cannot be used as a node id. + * + * `end` is the one models hit constantly, because it is the natural name for the last box in a + * flow. `default` is deliberately absent: it parses, so renaming it would be a change with no + * failure behind it. + */ +const RESERVED_NODE_IDS = new Set(['end', 'graph', 'class', 'style', 'subgraph', 'click']); + +/** Suffix appended when a reserved id has to be renamed. Unlikely to collide with a real id. */ +const RENAME_SUFFIX = '_node'; + +/** Characters that look like quotes to a model but are not the quote the grammar wants. */ +const SMART_QUOTES = /[\u2018\u2019\u201a\u201b\u2032]/g; +const SMART_DOUBLE_QUOTES = /[\u201c\u201d\u201e\u201f\u2033]/g; + +/** Whitespace that survives a copy-paste and is not the whitespace the lexer expects. */ +const EXOTIC_SPACES = /[\u00a0\u2000-\u200a\u202f\u205f\u3000]/g; +const ZERO_WIDTH = /[\u200b-\u200d\u2060\ufeff]/g; + +/** `
` in any of the spellings a model reaches for. */ +const BR_VARIANTS = /<\s*br\s*\/?\s*>/gi; + +/** A quoted label, used only once every label is known to hold no bare quotes. */ +const QUOTED_LABEL = /"([^"\n]*)"/g; + +/** A square node declaration: an id, a `[`, its text, and the first `]` after it. */ +const SQUARE_NODE = /([\w-]+)\[([^\]\n]*)\]/g; + +/** An edge label between pipes. */ +const EDGE_LABEL = /\|([^|\n]*)\|/g; + +/** An opening `subgraph` statement. */ +const SUBGRAPH_LINE = /^\s*subgraph\b/; + +/** A terminator, in the only spelling the grammar accepts and the ones it does not. */ +const END_LINE = /^\s*end\s*$/; +const MISCASED_END_LINE = /^\s*(END|End|eNd|enD|EnD|ENd|eND)\s*$/; + +/** + * Normalise the characters a model picked up from whatever it was reading. + * + * A leading byte-order mark is the sharpest of these: it makes the very first token + * `\ufeffflowchart` rather than `flowchart`, so mermaid reports that no diagram type was + * detected when the source is otherwise perfect. + */ +function normalizeCharacters(source: string): string { + return source + .replace(ZERO_WIDTH, '') + .replace(/\r\n?/g, '\n') + .replace(EXOTIC_SPACES, ' ') + .replace(SMART_QUOTES, "'") + .replace(SMART_DOUBLE_QUOTES, '"') + .replace(BR_VARIANTS, '
'); +} + +/** + * True for a line that is a comment rather than a statement. + * + * Used to leave alone any line the statement-level rules should not touch. + */ +function isCommentLine(line: string): boolean { + return /^\s*%%/.test(line); +} + +/** Placeholder standing in for `
` while the rest of a label is escaped around it. */ +const BR_PLACEHOLDER = '\u0001BR\u0001'; + +/** Delimiter for text held aside while a pattern runs over everything around it. */ +const STASH_OPEN = '\u0002'; + +/** Put stashed text back where it came from. */ +function unstash(text: string, stash: string[]): string { + return text.replace( + new RegExp(`${STASH_OPEN}(\\d+)${STASH_OPEN}`, 'g'), + (_match, index: string) => stash[Number(index)], + ); +} + +/** + * Escape everything in a label's text that the grammar or the sanitizer would take as syntax. + * + * A bare `"` is the sharpest of these: the string token runs to the next quote, so a label like + * `"He said "hello" loudly"` ends after `said ` and the remainder is parsed as syntax. Braces + * and angle brackets are escaped for the same reason, and because a model transcribing a header + * such as `x-ms-client-request-id: ` has no idea it is writing markup — mermaid's + * sanitizer would otherwise drop the placeholder entirely and lose the text. + * + * `
` is protected first. It is the one piece of markup a label is meant to contain, and it + * is what the guidance in functions_diagram_operations.py explicitly asks models to use. + */ +function escapeLabelText(text: string): string { + return text + .split('
') + .join(BR_PLACEHOLDER) + .replace(/&(?![a-zA-Z][a-zA-Z0-9]{1,10};|#\d{1,6};|#x[0-9a-fA-F]{1,5};)/g, '&') + .replace(/"/g, '"') + .replace(//g, '>') + .replace(/\{/g, '{') + .replace(/\}/g, '}') + .split(BR_PLACEHOLDER) + .join('
'); +} + +/** + * Rewrite one label's raw text into a quoted, escaped label. + * + * Returns null when nothing needs doing, which is what keeps the repair a no-op for a diagram + * that is merely a different kind of broken — and what lets `isRepairWorthTrying` answer + * honestly. + */ +function repairLabelBody(body: string): string | null { + const trimmed = body.trim(); + if (trimmed === '') { + // An empty label fails to parse. A single space keeps the node, unlabelled, rather + // than dropping it: it is still part of the answer the model gave. + return ' '; + } + + const quoted = trimmed.length >= 2 && trimmed.startsWith('"') && trimmed.endsWith('"'); + const inner = quoted ? trimmed.slice(1, -1) : trimmed; + + if (inner.trim() === '') { + return ' '; + } + + const escaped = escapeLabelText(inner); + if (quoted && escaped === inner) { + return null; + } + return escaped; +} + +/** + * Quote and escape node and edge labels. + * + * Only the square node form and the piped edge form are handled. Those are where every + * reproduced failure came from, and they are the two whose extent can be determined without + * guessing: a body containing the shape's own delimiter is skipped rather than mis-split. + * + * Square nodes are rewritten first and stashed. A pipe is legal inside a node label, and the + * edge-label pattern has no notion of quoting, so without stashing it would pair a pipe inside + * a label with the pipe that opens the real edge label and rewrite the arrow between them. + */ +function repairLabels(line: string): string { + const stash: string[] = []; + + const withNodes = line.replace(SQUARE_NODE, (_match, id: string, body: string) => { + let rebuilt: string; + if (body.includes('[')) { + rebuilt = `${id}[${body}]`; + } else { + const fixed = repairLabelBody(body); + rebuilt = fixed === null ? `${id}[${body}]` : `${id}["${fixed}"]`; + } + stash.push(rebuilt); + return `${STASH_OPEN}${stash.length - 1}${STASH_OPEN}`; + }); + + // An odd number of remaining pipes means they cannot all be label delimiters, so pairing + // them would be a guess. Left alone rather than rewritten wrongly. + const pipes = (withNodes.match(/\|/g) ?? []).length; + const withEdges = + pipes % 2 === 0 + ? withNodes.replace(EDGE_LABEL, (_match, body: string) => { + // `||` is an empty edge label in a flowchart, but it is also cardinality in + // an erDiagram. Left alone either way: there is nothing in it to repair. + if (body.trim() === '') { + return `|${body}|`; + } + const fixed = repairLabelBody(body); + return fixed === null ? `|${body}|` : `|"${fixed}"|`; + }) + : withNodes; + + return unstash(withEdges, stash); +} + +/** + * True when the source is a flowchart. + * + * Every rule below the character normalisation is flowchart grammar. `subgraph`, `end`, square + * node labels and piped edge labels all mean something else, or nothing, in the ten other + * diagram types mermaid supports, and rewriting a sequence diagram with flowchart rules would + * turn one failure into a different one. + */ +function isFlowchart(source: string): boolean { + for (const line of source.split('\n')) { + const trimmed = line.trim(); + if (trimmed === '' || trimmed.startsWith('%%')) { + continue; + } + return /^(flowchart|graph)\b/.test(trimmed); + } + return false; +} + +/** + * Rename node ids the grammar reserves. + * + * Every occurrence of the identifier is rewritten, not just its declaration, so the edges that + * referred to it still connect. Bounded to word boundaries, and skipped inside quoted labels, + * comments and `subgraph` terminators, so prose that merely contains the word is untouched. + * + * Excluding terminators is what makes `end` safe to rename at all. A lowercase `end` on its own + * line closes a `subgraph`; renaming those alongside a node called `end` would silently move + * everything that followed a terminator inside the group it was meant to close, and the result + * still parses, so the reader would be shown a diagram with the wrong structure rather than an + * error. + */ +function renameReservedIds(source: string): string { + const stash: string[] = []; + const hold = (text: string) => { + stash.push(text); + return `${STASH_OPEN}${stash.length - 1}${STASH_OPEN}`; + }; + + const stashed = source + .split('\n') + .map((line) => { + if (isCommentLine(line) || END_LINE.test(line)) { + return hold(line); + } + return line.replace(QUOTED_LABEL, (match) => hold(match)); + }) + .join('\n'); + + let repaired = stashed; + for (const reserved of RESERVED_NODE_IDS) { + // A declaration: the reserved word immediately followed by a shape opener. `subgraph` + // and `click` are keywords followed by a space, so only the shape form can be a node. + const declaration = new RegExp(`(^|[^\\w-])${reserved}(\\s*[\\[({])`, 'gm'); + if (!declaration.test(repaired)) { + continue; + } + repaired = repaired.replace( + new RegExp(`(^|[^\\w-])${reserved}(?![\\w-])`, 'gm'), + (_match, prefix: string) => `${prefix}${reserved}${RENAME_SUFFIX}`, + ); + } + + return unstash(repaired, stash); +} + +/** + * Fix the terminator's case and close any subgraph that was left open. + * + * The grammar accepts only lowercase `end`. An unclosed `subgraph` swallows everything after + * it, so the error mermaid reports points at the last line of the diagram rather than at the + * statement that is actually wrong — which makes it one of the harder failures to read. + */ +function balanceSubgraphs(source: string): string { + const lines = source.split('\n').map((line) => { + if (MISCASED_END_LINE.test(line)) { + return line.replace(/(END|End|eNd|enD|EnD|ENd|eND)/, 'end'); + } + return line; + }); + + let open = 0; + for (const line of lines) { + if (isCommentLine(line)) { + continue; + } + if (SUBGRAPH_LINE.test(line)) { + open += 1; + } else if (END_LINE.test(line) && open > 0) { + open -= 1; + } + } + + for (let index = 0; index < open; index += 1) { + lines.push('end'); + } + + return lines.join('\n'); +} + +/** + * Drop a statement that names a source but no target. + * + * A reply truncated mid-diagram ends in a dangling `A -->`, which fails to parse and takes the + * whole diagram with it. The rest of the diagram is still worth drawing. + */ +function dropDanglingEdges(source: string): string { + return source + .split('\n') + .filter((line) => !/^\s*[\w-]+\s*(-{2,}>?|={2,}>?|-\.->?)\s*$/.test(line)) + .join('\n'); +} + +/** Split node declarations that a model ran together onto one line. */ +function splitRunTogetherStatements(source: string): string { + return source + .split('\n') + .map((line) => { + if (isCommentLine(line) || /-->|---|-\.-|==>/.test(line)) { + return line; + } + const indent = /^\s*/.exec(line)?.[0] ?? ''; + return line.replace( + /(\]|\)|\})\s+(?=[\w-]+\s*[\[({])/g, + (_match, close: string) => `${close}\n${indent}`, + ); + }) + .join('\n'); +} + +/** + * A best-effort rewrite of diagram source mermaid has already rejected. + * + * Pure and synchronous, so it can be unit tested without a browser. Returns the source + * unchanged when nothing applies, which the caller uses to skip a pointless second render. + */ +export function repairMermaidSource(source: string): string { + if (!source) { + return source; + } + + const normalized = normalizeCharacters(source); + if (!isFlowchart(normalized)) { + return normalized.trim(); + } + + let repaired = splitRunTogetherStatements(normalized); + repaired = repaired + .split('\n') + .map((line) => (isCommentLine(line) ? line : repairLabels(line))) + .join('\n'); + repaired = renameReservedIds(repaired); + repaired = balanceSubgraphs(repaired); + repaired = dropDanglingEdges(repaired); + + return repaired.trim(); +} + +/** True when repairing would produce something different, so a retry is worth attempting. */ +export function isRepairWorthTrying(source: string): boolean { + const trimmed = source.trim(); + return repairMermaidSource(trimmed) !== trimmed; +} + +/** + * A short, readable reason a diagram could not be drawn. + * + * Mermaid's own messages are multi-line parser dumps with a caret diagram in them. The first + * line carries the useful part; the rest belongs in the details disclosure, not in a summary. + * The two limit errors are recognised and reworded, because "Edge limit exceeded" tells a + * reader nothing about what to do next. + */ +export function describeMermaidError(error: unknown): string { + const raw = + error instanceof Error + ? error.message + : typeof error === 'string' + ? error + : ''; + const text = raw.trim(); + + if (!text) { + return 'The diagram could not be drawn.'; + } + if (/edge limit exceeded/i.test(text)) { + return 'The diagram has too many connections to draw.'; + } + if (/maximum text size/i.test(text)) { + return 'The diagram source is too large to draw.'; + } + if (/no diagram type detected/i.test(text)) { + return 'The first line does not name a diagram type mermaid recognises.'; + } + + const firstLine = text.split('\n', 1)[0].trim(); + return firstLine.length > 200 ? `${firstLine.slice(0, 197)}…` : firstLine; +} diff --git a/application/v2_ui/src/lib/vendor.d.ts b/application/v2_ui/src/lib/vendor.d.ts index 72e44997d..129e6b368 100644 --- a/application/v2_ui/src/lib/vendor.d.ts +++ b/application/v2_ui/src/lib/vendor.d.ts @@ -57,7 +57,17 @@ export interface MermaidConfig { fontFamily?: string; /** False keeps labels as SVG text rather than embedded foreignObject HTML. */ htmlLabels?: boolean; - flowchart?: { htmlLabels?: boolean; useMaxWidth?: boolean }; + flowchart?: { + htmlLabels?: boolean; + useMaxWidth?: boolean; + /** + * Pixel width a node label wraps at. + * + * Mermaid's default of 200 turns the long labels models write into narrow columns of + * text, which makes a diagram taller and harder to read rather than shorter. + */ + wrappingWidth?: number; + }; sequence?: { useMaxWidth?: boolean }; gantt?: { useMaxWidth?: boolean }; class?: { htmlLabels?: boolean; useMaxWidth?: boolean }; diff --git a/application/v2_ui/src/stores/chatStore.ts b/application/v2_ui/src/stores/chatStore.ts index c87b97ef1..2a413769e 100644 --- a/application/v2_ui/src/stores/chatStore.ts +++ b/application/v2_ui/src/stores/chatStore.ts @@ -177,6 +177,13 @@ interface ChatState { blockIndex: number, sourceHash: string, style: VisualStyle | null, + /** + * The block's stage height in pixels. + * + * `undefined` leaves whatever is stored alone, so a colour change does not reset a + * size someone chose; `null` clears it back to the automatic height. + */ + height?: number | null, ) => Promise; sendFeedback: ( messageId: string, @@ -1174,6 +1181,7 @@ export const useChatStore = create((set, get) => ({ blockIndex, sourceHash, style, + height, ) => { if (!conversationId) { return false; @@ -1192,6 +1200,9 @@ export const useChatStore = create((set, get) => ({ colors: style.colors, } : null, + // Spread rather than always sent: the server distinguishes an absent key, + // which keeps the stored height, from an explicit null, which clears it. + ...(height === undefined ? {} : { height }), }); set((state) => ({ diff --git a/docs/explanation/features/MERMAID_DIAGRAM_RENDERING.md b/docs/explanation/features/MERMAID_DIAGRAM_RENDERING.md index 06b82d536..da7636173 100644 --- a/docs/explanation/features/MERMAID_DIAGRAM_RENDERING.md +++ b/docs/explanation/features/MERMAID_DIAGRAM_RENDERING.md @@ -107,6 +107,49 @@ The V2 interface solves the same problem the same way inside `MermaidDiagram.tsx The V2 interface needed no rendering change. It picks up the prompt guidance and starts receiving diagrams it could already draw. +## Recovering from a diagram that will not parse + +Models get Mermaid wrong in a small number of predictable ways, and one bad character takes the +whole diagram with it. The V2 renderer therefore repairs and retries before giving up. Each of +the following was reproduced as a real parse failure against the vendored Mermaid 11.17.2 bundle +in Chromium, and each renders after repair: + +| What the model wrote | Why it failed | +|---|---| +| `end["End"]` as a node id | `end` is reserved; so are `graph`, `class` and `style` | +| `End` closing a `subgraph` | Only lowercase `end` is accepted | +| A `subgraph` with no terminator | Swallows the rest of the diagram, and the error points at the last line | +| `a[""]` | An empty label does not parse | +| `a[App (main)]` | Unquoted parentheses | +| `a -->|metadata: {}| b` | Unquoted braces in an edge label | +| `a["He said "hello" loudly"]` | The string token ends at the second quote | +| `a["A"] b["B"]` | Two declarations on one line | +| A trailing `b -->` | A truncated reply leaves a dangling edge | +| A leading byte-order mark | Reported as "no diagram type detected" | + +Two rules keep this safe: + +- **The repair only ever runs after Mermaid has already refused the source.** A diagram that + parses is handed over untouched, so nothing that renders today can be changed by it. +- **It is scoped to flowcharts.** `subgraph`, `end`, square labels and piped edge labels mean + something else in the other diagram types — `||--o{` in an `erDiagram`, for instance — so any + other diagram type is left alone. + +When repair does not help, the panel shows the source with the reason behind **Show details**, +and the reason is written to the browser console. Mermaid's limit errors are reworded: "Edge +limit exceeded. 500 edges found, but the limit is 500." becomes "The diagram has too many +connections to draw." + +The prompt guidance was extended to head off the same failures at the source: it names the +reserved words, requires a lowercase `end` for every `subgraph`, tells the model not to carry +placeholders such as `` out of pasted text into a label, and asks for short labels +across several nodes instead of one node holding a dozen `
` lines. + +## Viewing a diagram in the V2 interface + +Sizing, zoom, the drag-to-resize handle and the full-screen viewer are described in +[V2 Diagram And Chart Styling](V2_DIAGRAM_AND_CHART_STYLING.md). + ## Testing and validation `functional_tests/test_mermaid_diagram_prompt_guidance.py` covers diagram intent detection @@ -119,6 +162,11 @@ headless Chromium: it renders a real diagram and asserts its labels survive, che pending-fence placeholder, the source fallback for an unparseable diagram, the render cache, teardown, and that no request leaves the origin. +`functional_tests/test_v2_diagram_viewer_controls.py` and its bundled +`test_v2_diagram_viewer_logic.ts` cover the source repair, the error reporting, the render +timeout and size guard, and the V2 viewer controls. The load-bearing assertion is that the +repair is a no-op for every diagram Mermaid already accepts. + `functional_tests/test_export_mermaid_browser_rasterizer.py` and `functional_tests/test_export_mermaid_server_render.py` continue to assert that the browser and server renderers agree on configuration, now reading the browser side from the shared @@ -127,9 +175,13 @@ runtime. ## Known limitations Diagram quality depends on the model. The guidance improves the odds that a diagram parses -and is well chosen, but a model can still emit invalid Mermaid; that case shows the source +and is well chosen, and the repair pass recovers the common failures, but a model can still +emit Mermaid that cannot be salvaged; that case shows the source and the parser's reason rather than failing silently. +Some failures cannot be repaired at all and are reported instead. A diagram beyond Mermaid's +500-edge limit is one — it would measure over 50,000 pixels tall even if it parsed. + A ` ```mermaid ` fence nested inside a larger fenced block is still treated as a diagram. This matches the existing behaviour of inline chart blocks. diff --git a/docs/explanation/features/V2_DIAGRAM_AND_CHART_STYLING.md b/docs/explanation/features/V2_DIAGRAM_AND_CHART_STYLING.md index 8270fe7c3..2b95ac6aa 100644 --- a/docs/explanation/features/V2_DIAGRAM_AND_CHART_STYLING.md +++ b/docs/explanation/features/V2_DIAGRAM_AND_CHART_STYLING.md @@ -137,11 +137,65 @@ refusing to allocate the canvas. text rather than embedded HTML — and it is also what makes a diagram rasterizable at all, since a `` label disappears when an SVG is painted onto a canvas. +## Sizing a diagram + +A diagram's panel takes its width from the diagram's own measured natural width, read back out of +the `max-width` Mermaid writes. This matters more than it sounds: the assistant bubble is +shrink-to-fit, and Mermaid emits `width="100%"`, which contributes nothing to intrinsic sizing. +Without a measured width the bubble collapsed to the width of the diagram's toolbar and the +diagram was drawn at roughly a quarter of its natural size. + +The stage the diagram sits in is capped at 520 pixels tall by default and scrolls internally. +Without a cap a large flowchart goes straight into the message list — one at Mermaid's own limit +of 500 edges measures over 50,000 pixels tall — where the browser re-rasterizes it on every +scroll frame. + +Three controls change what you see: + +| Control | Effect | +|---|---| +| `−` / `+` | Scales the diagram between 0.4x and 4x of the scale that fits it to the panel | +| Fit | Returns to fitting the panel width | +| **Expand** | Opens the diagram full screen, with its own zoom and PNG download | + +The bar along the bottom edge of the stage is a drag handle. It is exposed as a slider, so it can +be moved with the arrow keys and reset to the automatic height with **Home**. The height you +leave it at is saved on the message alongside the colours, and the two are independent: resetting +the colours does not resize the diagram, and resizing it does not stop it following your default +palette. + +Mermaid's `flowchart.wrappingWidth` is set to 500 rather than its default of 200. The default +wraps the long labels models write into narrow columns of text, which makes a diagram taller and +harder to read: the same diagram measures 273 x 955 at 200 and 497 x 867 at 500. + +## When a diagram will not render + +A diagram that Mermaid rejects falls back to its source, because the source is still the answer +the model gave. The panel says why, behind **Show details**, and offers **Copy source**. The +reason also goes to the browser console. + +Before the reader sees any of that, the source is repaired and rendered once more. +`repairMermaidSource()` fixes the mistakes models actually make — reserved words used as node +ids, a capitalised `End`, an unclosed `subgraph`, an empty label, unquoted parentheses or braces, +a bare quote inside a label, two statements run onto one line, a dangling edge, and stray +byte-order marks, non-breaking spaces and smart quotes. + +Two properties of the repair matter: + +- **It only runs after a failure.** A diagram Mermaid accepts is handed over untouched and can + never be rewritten, so nothing that renders today can change. +- **It is scoped to flowcharts.** `subgraph`, `end`, square labels and piped edge labels all mean + something else in the other diagram types — `||--o{` in an `erDiagram`, for one — so anything + that is not a flowchart is left alone. + +Rendering is bounded either way: a diagram is given 10 seconds, and a source longer than 30,000 +characters is refused with its own message rather than being attempted. + ## API ### `POST /api/message//visual-style` -Saves or clears the colours for one block. +Saves or clears the colours, and the chosen height, for one block. ```json { @@ -153,13 +207,22 @@ Saves or clears the colours for one block. "palette": "vivid", "background": "#ffffff", "colors": { "0": "#123456" } - } + }, + "height": 640 } ``` -A `style` of `null` removes the entry, which is not the same as saving a style that happens to +A `style` of `null` removes the colours, which is not the same as saving a style that happens to equal your current default: a removed entry follows the default when the default later changes. +`height` is optional and independent of `style`. **Omitting the key** leaves whatever height is +stored alone, so changing colours never resets a diagram you resized; **sending `null`** clears +it so the block is sized automatically again. It is validated as a number and clamped to +140–2000 pixels rather than refused, because it arrives from a drag and a value a few pixels past +the limit is someone holding the mouse down, not a client misbehaving. + +An entry is removed entirely only once it holds neither colours nor a height. + The response returns the whole stored map for the message, not just the entry that changed: ```json @@ -167,11 +230,15 @@ The response returns the whole stored map for the message, not just the entry th "success": true, "message_id": "msg-456", "visual_styles": { - "simplechart": { "1": { "palette": "vivid", "background": "#ffffff", "colors": {}, "source_hash": "a1b2c3d4" } } + "simplechart": { "1": { "palette": "vivid", "background": "#ffffff", "colors": {}, "source_hash": "a1b2c3d4", "height": 640 } } } } ``` +An entry carrying only a `height` is **not** a colour override. The client tells the two apart by +the presence of `palette`, so a diagram you only resized still follows your default palette +rather than being pinned to the built-in one. + The endpoint authorizes the **conversation**, not the message. A diagram lives in an assistant message, which carries no author of its own, and authorizing the conversation also admits a participant acting inside a shared conversation. @@ -200,7 +267,8 @@ so the accepted form is deliberately narrow: - Unknown keys in a submitted style are dropped rather than stored, so a client ahead of the server cannot put arbitrary fields into a message document. - Sizes are capped: block index 0–199, 24 colour overrides per block, 100 styled blocks per - message. A message document cannot be grown without bound by repeated requests. + message, and a block height of 140–2000 pixels. A message document cannot be grown without + bound by repeated requests. The rendering guarantees that were already in place are unchanged. Mermaid still runs at `securityLevel: 'strict'` with `htmlLabels: false`, its output still passes through DOMPurify @@ -212,9 +280,11 @@ as an independent second boundary, and `bindFunctions` is still never called. |---|---| | `application/v2_ui/src/lib/visualPalettes.ts` | Presets, colour maths, Mermaid theme mapping, fingerprint, resolver | | `application/v2_ui/src/lib/svgRaster.ts` | SVG element to PNG data URI | -| `application/v2_ui/src/lib/blockVisualStyle.ts` | Hook resolving and saving one block's style | +| `application/v2_ui/src/lib/blockVisualStyle.ts` | Hook resolving and saving one block's colours and height | +| `application/v2_ui/src/lib/mermaidSource.ts` | Repairs and describes diagram source Mermaid has rejected | | `application/v2_ui/src/components/chat/VisualStyleMenu.tsx` | The shared colour controls | -| `application/v2_ui/src/components/chat/MermaidDiagram.tsx` | Diagram rendering, toolbar and PNG download | +| `application/v2_ui/src/components/chat/MermaidDiagram.tsx` | Diagram rendering, toolbar, PNG download and the full-screen viewer | +| `application/v2_ui/src/components/chat/DiagramStage.tsx` | Natural-size measurement, the bounded stage and the resize handle | | `application/v2_ui/src/components/chat/InlineChart.tsx` | Chart rendering and toolbar | | `application/v2_ui/src/lib/richBlocks.ts` | Fence languages and the streaming placeholder guard | | `application/v2_ui/src/lib/rehypeRichBlockIndex.ts` | Numbers the blocks on the parsed tree | @@ -229,11 +299,15 @@ as an independent second boundary, and `bindFunctions` is still never called. - `functional_tests/test_v2_visual_style_logic.ts` — behavioural checks of the colour and fence-numbering logic, bundled with esbuild and run by the test above when the front-end toolchain is installed. +- `functional_tests/test_v2_diagram_viewer_controls.py` and + `functional_tests/test_v2_diagram_viewer_logic.ts` — sizing, the stage cap, the resize handle, + the expanded viewer, height storage, and the source repair. The most important assertion is a negative one: a block nobody has recoloured produces byte-identical Chart.js configuration to before this feature existed, and a diagram nobody has recoloured keeps Mermaid's stock `default` or `dark` theme. Existing conversations are -unaffected. +unaffected. The repair pass is held to the same standard — it must be a no-op for every diagram +Mermaid already accepts. ## Known limitations diff --git a/docs/explanation/fixes/V2_DIAGRAM_VIEWER_FIX.md b/docs/explanation/fixes/V2_DIAGRAM_VIEWER_FIX.md new file mode 100644 index 000000000..d0ddd0f10 --- /dev/null +++ b/docs/explanation/fixes/V2_DIAGRAM_VIEWER_FIX.md @@ -0,0 +1,204 @@ +# V2 Diagram Viewer Fix + +Diagrams in the V2 chat rendered too small to read, made long threads unusable, and sometimes +did not render at all with nothing to explain why. + +Fixed in version: **0.261.037** + +## The reports + +1. Diagrams render "really tiny, kind of hard to see" — but clicking **Colors** makes the same + diagram large enough to read. +2. A long diagram makes the chat "reload" when scrolling, and the bottom of the thread becomes + unreachable. +3. Some diagrams show **Diagram could not be rendered** and produce no logs at all. +4. There is no way to make a diagram bigger. + +## Root causes + +Each was reproduced against the vendored mermaid 11.17.2 bundle in Chromium, configured exactly +as `MermaidDiagram.tsx` configures it, before anything was changed. + +### 1 and the Colors jump: the diagram contributed no width + +The assistant bubble is a shrink-to-fit flex item — `bubbleWidthClass()` supplies a `max-width` +and nothing else, so its width is `min(max-content of its contents, max-width)`. + +Mermaid renders with `useMaxWidth: true`, which emits: + +```html + +``` + +A percentage width contributes essentially nothing to intrinsic sizing, so a message containing +only a diagram collapsed the bubble to the width of the diagram's own toolbar, and the +`width: 100%` SVG then scaled itself down to match. + +Opening the **Colors** menu revealed the same bug from the other side: `PalettePresets` is a +wrapping row of five labelled swatch buttons, which *does* have a natural width, so the bubble +grew and the diagram grew with it. + +Measured in Chromium, using the natural size of the Azure governance diagram from the report: + +| | Panel | Diagram drawn at | +|---|---|---| +| Before | 358px | 300px — 27% of natural size | +| Before, Colors open | 575px | 517px — 47% of natural size | +| After | 1024px | 966px — 88% of natural size | +| After, Colors open | 1024px | unchanged | + +### 2: nothing bounded a diagram's height + +The diagram stage had no height cap. A flowchart at mermaid's own default limit of 500 edges +measures **50,466 pixels tall**, and that went straight into the message list, where the browser +re-rasterizes it on every scroll frame. + +Two further faults compounded it: + +- `MessageBubble` was not memoised, and `readMaskState()` ran unmemoised on every render. The + list re-rendered on every streaming token *and* every time the scroll crossed the + pinned-to-bottom threshold, so the entire remark/rehype pipeline re-ran for every message in + the thread each time. +- The auto-scroll effect ran on `[messages, streamingContent, pinnedToBottom]`. A diagram + renders **asynchronously**: a 96px placeholder is replaced by a much taller panel long after + the scroll that was meant to land at the bottom. Nothing re-ran, so the bottom stayed out of + reach. + +### 3: the error was thrown away + +```ts +.catch(() => { + if (!cancelled) { + setState({ status: 'error' }); // the error object is never read + } +}); +``` + +No message, no `console.warn`, nothing in the panel beyond the words "Diagram could not be +rendered". The classic client at least logged the error +(`chat-inline-diagrams.js`). There was also no render timeout and no source-size guard, both of +which the classic client has. + +Twelve distinct parse failures were reproduced, all of them things models actually write: + +| Source | Mermaid's response | +|---|---| +| `end["End"]` as a node id | Parse error — `end` is reserved | +| `graph`, `class`, `style` as node ids | Parse error — all reserved | +| `End` closing a subgraph | Parse error — only lowercase `end` is accepted | +| A `subgraph` with no `end` | Parse error, reported at the last line of the diagram | +| `a[""]` | Parse error | +| `a[App (main)]` | Parse error | +| `a -->|metadata: {}| b` | Parse error | +| `a["He said "hello" loudly"]` | Parse error — the string token ends at the second quote | +| `a["A"] b["B"]` on one line | Parse error | +| A trailing `b -->` with no target | Parse error | +| A leading byte-order mark | "No diagram type detected" | +| 500 edges | "Edge limit exceeded" | + +### An additional finding: label wrapping + +Mermaid's `flowchart.wrappingWidth` defaults to 200px, which turns the long labels models write +into narrow columns of text — making a diagram *taller* and harder to read. The same +label-heavy diagram measures: + +| `wrappingWidth` | Natural size | +|---|---| +| 200 (mermaid's default) | 273 x 955 | +| 500 | 497 x 867 | + +## Files modified + +| File | Change | +|---|---| +| `application/v2_ui/src/components/chat/MermaidDiagram.tsx` | Error reporting, render timeout, source guard, repair-and-retry, zoom, expanded viewer, panel width from measured natural size | +| `application/v2_ui/src/components/chat/DiagramStage.tsx` | New. Natural-size measurement, bounded scrolling stage, resize handle | +| `application/v2_ui/src/lib/mermaidSource.ts` | New. `repairMermaidSource()`, `isRepairWorthTrying()`, `describeMermaidError()` | +| `application/v2_ui/src/components/chat/MessageList.tsx` | Memoised bubbles, pinned flag moved to a ref, `ResizeObserver` re-pin | +| `application/v2_ui/src/lib/blockVisualStyle.ts` | Reads and writes a stored height alongside the colours | +| `application/v2_ui/src/lib/endpoints.ts`, `src/stores/chatStore.ts` | Optional `height` on the visual-style request | +| `application/single_app/functions_message_visual_styles.py` | Validates, clamps and stores `height` independently of the colours | +| `application/single_app/route_backend_chats.py` | Accepts `height`, distinguishing "absent" from "null" | +| `application/single_app/functions_diagram_operations.py` | Guidance covering reserved words, terminators, placeholders and label length | + +## What changed in behaviour + +**Sizing.** The panel takes its width from the diagram's measured natural width, floored so the +toolbar never wraps and capped at the bubble. Because the width is now definite, the Colors menu +can no longer resize anything. + +**Height.** The stage is capped at 520px by default and scrolls internally, so a tall diagram is +a panel rather than a wall. A drag handle on its bottom edge sets a different height; it is a +slider, so it works from the keyboard, with **Home** returning to the automatic height. The +chosen height is stored on the message beside the colours. + +**Zoom and expand.** `−`, `+` and a fit control scale the diagram between 0.4x and 4x of the +fit-to-width scale. **Expand** opens a full-screen viewer with its own zoom and PNG download, +following the same conventions as the existing image lightbox. + +**Failure reporting.** The reason is kept, logged with `console.warn`, and shown in the fallback +panel behind **Show details**. The panel also offers **Copy source**. Mermaid's limit errors are +reworded — "Edge limit exceeded. 500 edges found, but the limit is 500." becomes "The diagram has +too many connections to draw." + +**Repair and retry.** When mermaid rejects a diagram, `repairMermaidSource()` rewrites it and it +is tried once more. The repair runs **only after a failure**, so a diagram that renders today is +handed to mermaid untouched and can never be changed by it. It is also scoped to flowcharts: +`subgraph`, `end`, square labels and piped edge labels all mean something else in the other +diagram types, and `||--o{` in an `erDiagram` must survive intact. + +## Validation + +`functional_tests/test_v2_diagram_viewer_controls.py` — 18 checks, including 58 bundled +TypeScript behaviour checks in `test_v2_diagram_viewer_logic.ts`. + +Verified in Chromium against the real mermaid bundle: + +- All 14 reproduced parse failures render after repair. +- All 6 working diagrams — flowchart, sequence, state, class, ER, and the two from the report — + are byte-identical after repair, so `isRepairWorthTrying()` returns false and no second render + is attempted. +- The layout measurements in the table above. +- `DiagramStage` mounted directly: fit scale, zoom, resize, clamping and reset all measured. + +Existing suites re-run and passing: `test_v2_visual_style_controls.py` (16), +`test_v2_rich_rendering.py` (13), `test_chat_inline_diagram_rendering.py` (5), +`test_mermaid_diagram_prompt_guidance.py` (7), `test_export_mermaid_server_render.py` (10), +`test_conversation_export_mermaid_tex_images.py` (18), +`test_export_mermaid_browser_rasterizer.py` (3), and the three route policy suites. + +### Issues caught during review + +Four defects were found by review and fixed before this shipped. They are recorded because each +was a silent failure rather than a visible one: + +1. **A stored height survived a source change.** `apply_visual_style` carried the previous + entry's height forward without comparing its `source_hash` to the incoming one, then stamped + the entry with the *new* fingerprint. A height chosen for a block that an edit had shifted out + of that position was resurrected and made authoritative for different content — defeating the + fingerprint guard the client already honours. +2. **Renaming a reserved id ate the subgraph terminators.** `end` is both the most common + reserved node id models use and the keyword that closes a `subgraph`. Renaming every + occurrence rewrote the terminators too, so `balanceSubgraphs` then appended one at the bottom + and everything after the original terminator was swallowed into the group. The result still + parsed, so it showed the wrong structure rather than an error. +3. **A pipe inside a node label was paired with the edge-label pipe.** `a["A|B"] --> |"yes"| b` + had the arrow itself escaped into the middle of a label, which could destroy a line that was + not the reason the diagram failed. Square node labels are now stashed before the edge pass + runs, and a line with an odd number of pipes is left alone rather than paired by guesswork. +4. **`Infinity` as a height returned a 500.** `json.loads` accepts the bare `Infinity` token and + `round(float('inf'))` raises `OverflowError`, which is not a `VisualStyleError` and so escaped + the route's handler. Now rejected with a 400. + +Each has a regression test. + +## Notes + +No new npm package and no remote asset. Mermaid and DOMPurify were already vendored locally, so +the `default-src 'self'` Content-Security-Policy is unchanged. + +The full-screen viewer is defined inside `MermaidDiagram.tsx` rather than in its own file. It +writes diagram markup to the DOM, and +`test_v2_rich_rendering.py::test_sanitizer_boundary_at_every_html_sink` fixes the set of +components allowed to do that; keeping it in the reviewed file preserves that invariant instead +of widening it. diff --git a/docs/explanation/release_notes.md b/docs/explanation/release_notes.md index 0838c1b22..0e3512bc9 100644 --- a/docs/explanation/release_notes.md +++ b/docs/explanation/release_notes.md @@ -2,6 +2,42 @@ For feature-focused and fix-focused drill-downs by version, see [Features by Version](https://github.com/microsoft/simplechat/tree/main/docs/explanation/features) and [Fixes by Version](https://github.com/microsoft/simplechat/tree/main/docs/explanation/fixes). +### **(v0.261.037)** + +#### Bug Fixes + +* **Diagrams No Longer Render Too Small To Read** + * A diagram in the new interface was drawn at roughly a quarter of its natural size — and clicking **Colors** made it suddenly readable. Both were the same bug: the message bubble sizes itself to its contents, and a diagram was telling it "I'll take whatever width you have", so the bubble shrank to the width of the diagram's own buttons. Opening the colour menu added something that *did* have a width, which is why the diagram grew. + * A diagram now sizes its own panel. Measured on the Azure hierarchy diagram from the report, it went from 300 pixels wide to 966, and opening **Colors** no longer changes anything. + * Long labels also wrap at a more sensible width, so a diagram with wordy boxes is no longer squeezed into tall, narrow columns of text. + * (Ref: V2 chat, Mermaid diagrams, message layout) + +* **A Long Diagram No Longer Breaks Scrolling** + * A tall diagram used to be dropped into the conversation at full height — a large flowchart can be tens of thousands of pixels tall — which made scrolling stutter or lock up. A diagram now sits in a panel of its own that scrolls internally. + * The bottom of a conversation was also unreachable after a diagram appeared: diagrams draw a moment after the message does, so the chat had already scrolled before the diagram grew. It now follows content that arrives late. + * Scrolling and typing in a long conversation are noticeably lighter, because the messages you are not looking at are no longer redrawn on every scroll and every word of a streaming reply. + * (Ref: V2 chat, Mermaid diagrams, message list performance) + +* **A Diagram That Will Not Draw Now Says Why** + * "Diagram could not be rendered" was all you got, and nothing was written to the browser console either, so there was nothing to report and nothing to look into. The reason is now shown behind **Show details**, logged to the console, and the source can be copied with one click. + * More usefully, most of those diagrams now just work. When a diagram fails, SimpleChat repairs the common mistakes and draws it again: reserved words such as `end` used as a box name, a `subgraph` left unclosed, `End` instead of `end`, placeholders like `` carried over from pasted text, stray quotes and braces inside a label, and several more. Fourteen distinct failures were reproduced and all fourteen now render. + * Repairs only happen after a diagram has already failed, so a diagram that draws correctly today is never altered. + * A diagram is also given a time limit and a size limit, so a broken one can no longer leave "Rendering diagram…" on screen indefinitely. + * (Ref: V2 chat, Mermaid diagrams, diagram source repair) + +#### User Interface Enhancements + +* **Make A Diagram Bigger** + * Diagrams now have **−**, **+** and a fit button to scale them, and an **Expand** button that opens the diagram full screen with its own zoom and PNG download. + * The bar along the bottom edge of a diagram can be dragged to make the panel taller or shorter. It also works from the keyboard — arrow keys to resize, **Home** to go back to the automatic height. + * The size you leave a diagram at is remembered with the conversation, so it is still there when you come back. Resizing a diagram does not disturb its colours, and changing its colours does not resize it. + * (Ref: V2 chat, Mermaid diagrams, diagram zoom and resize) + +* **Better Diagrams From The Assistant** + * The assistant is now told which words break a diagram, to close every `subgraph`, and to keep box labels to a short phrase rather than packing a dozen lines into one box — which is what made the reported diagrams both unreadable and unrenderable. + * When you paste text or ASCII art and ask for a diagram, it is told to summarise the detail rather than copying placeholders and punctuation straight into the boxes. + * (Ref: diagram prompt guidance) + ### **(v0.261.036)** #### Bug Fixes diff --git a/functional_tests/test_v2_diagram_viewer_controls.py b/functional_tests/test_v2_diagram_viewer_controls.py new file mode 100644 index 000000000..fdb6dfd4a --- /dev/null +++ b/functional_tests/test_v2_diagram_viewer_controls.py @@ -0,0 +1,520 @@ +#!/usr/bin/env python3 +""" +Functional test for the V2 diagram viewer: sizing, resizing, expanding and render recovery. + +Version: 0.261.037 +Implemented in: 0.261.037 + +Four reported problems, each reproduced against the vendored mermaid 11.17.2 bundle in +Chromium before anything was changed: + + - **Diagrams rendered far too small, and jumped larger when the colour menu was opened.** + The assistant bubble is shrink-to-fit and mermaid emits ``width="100%"``, which contributes + nothing to intrinsic sizing, so the bubble collapsed to the width of the diagram's own + toolbar. Opening the colour menu introduced a palette row that *does* have a natural width, + which is why the same diagram suddenly became legible. The panel now takes its width from + the diagram's measured natural width. + + - **A long diagram made the thread unusable.** A flowchart at mermaid's default limit of 500 + edges measures 50,466 pixels tall. With no cap it went straight into the scroll container. + The stage now has a bounded height and scrolls internally. + + - **Some diagrams never rendered, with nothing to go on.** The error was caught and + discarded: no message, no console entry. It is now shown, logged, and the source is + repaired and retried once before the reader is given up on. + + - **There was no way to make a diagram bigger.** Zoom, a drag-to-resize handle whose height is + kept on the message, and a full-screen viewer. + +The strongest assertion here is negative: ``repairMermaidSource`` must be a no-op for every +diagram mermaid already accepts. It only ever runs after a failure, so rewriting working output +would be a regression with nothing gained. +""" + +import re +import subprocess +import sys +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[1] +APP_DIR = REPO_ROOT / "application" / "single_app" +V2_SRC = REPO_ROOT / "application" / "v2_ui" / "src" + +sys.path.insert(0, str(REPO_ROOT / "functional_tests")) +sys.path.insert(0, str(APP_DIR)) + +from test_support.versioning import assert_app_version_at_least # noqa: E402 + +from functions_message_visual_styles import ( # noqa: E402 + MAX_BLOCK_HEIGHT, + MIN_BLOCK_HEIGHT, + UNSET, + VisualStyleError, + apply_visual_style, + read_visual_styles, + validate_block_height, +) + +IMPLEMENTED_IN = "0.261.037" + +MERMAID_TSX = V2_SRC / "components" / "chat" / "MermaidDiagram.tsx" +STAGE_TSX = V2_SRC / "components" / "chat" / "DiagramStage.tsx" +MESSAGE_LIST_TSX = V2_SRC / "components" / "chat" / "MessageList.tsx" +SOURCE_TS = V2_SRC / "lib" / "mermaidSource.ts" +BLOCK_STYLE_TS = V2_SRC / "lib" / "blockVisualStyle.ts" + + +def _read(path): + return path.read_text(encoding="utf-8", errors="ignore") + + +def test_version_is_at_least_the_implementing_release(): + """The fix is present from the version it was implemented in onwards.""" + assert_app_version_at_least(IMPLEMENTED_IN) + print(" ok application version is at or beyond the implementing release") + + +def test_the_panel_takes_its_width_from_the_diagram(): + """The diagram sizes the panel, rather than the panel sizing the diagram.""" + source = _read(MERMAID_TSX) + + assert "MIN_FIGURE_WIDTH" in source, ( + "the panel needs a floor so a narrow diagram does not squeeze the toolbar" + ) + assert re.search(r"width:\s*Math\.max\(size\.width,\s*MIN_FIGURE_WIDTH\)", source), ( + "the figure must be given the diagram's measured natural width. Without a definite " + "width the shrink-to-fit assistant bubble collapses to the toolbar and mermaid's " + "width:100% SVG is drawn illegibly small." + ) + assert "maxWidth: '100%'" in source, ( + "a diagram wider than the bubble must be held inside it rather than overflowing" + ) + + stage = _read(STAGE_TSX) + assert "max-width:\\s*([0-9.]+)px" in stage or "max-width:" in stage, ( + "the natural width is read back out of the max-width mermaid emits" + ) + print(" ok the panel is sized from the diagram's natural width") + + +def test_a_long_diagram_cannot_fill_the_thread(): + """The stage is bounded and scrolls, so a tall diagram stays a panel.""" + stage = _read(STAGE_TSX) + + assert "DEFAULT_MAX_STAGE_HEIGHT" in stage, "a default height ceiling must exist" + assert "MAX_STAGE_HEIGHT" in stage and "MIN_STAGE_HEIGHT" in stage, ( + "the resize handle must have bounds" + ) + assert "overflow-auto" in stage, "the stage must scroll rather than grow without limit" + assert "[contain:content]" in stage, ( + "paint containment is what stops a large diagram re-rasterizing the thread on scroll" + ) + print(" ok a long diagram is capped and scrolls inside its own panel") + + +def test_the_render_failure_is_reported_rather_than_swallowed(): + """A diagram that will not draw says why, in the panel and in the console.""" + source = _read(MERMAID_TSX) + + assert "console.warn(" in source, ( + "the parser's own words must reach the console; the previous `.catch(() => ...)` " + "discarded the error entirely, which is why a failure could not be diagnosed" + ) + assert "describeMermaidError(" in source, "the reason must be turned into something readable" + assert re.search(r"status:\s*'error';\s*reason:\s*string", source), ( + "the error state must carry the reason, not just the fact of failure" + ) + assert "Show details" in source, "the reason must be reachable from the fallback panel" + assert "Copy source" in source, "the source must be copyable when it cannot be drawn" + print(" ok a render failure reports why, in the panel and the console") + + +def test_rendering_is_bounded(): + """A render cannot hang, and an oversized source is refused with its own message.""" + source = _read(MERMAID_TSX) + + assert "RENDER_TIMEOUT_MS = 10000" in source, ( + "matches MERMAID_RENDER_TIMEOUT_MS in chat-mermaid-runtime.js; without it a wedged " + "render leaves 'Rendering diagram…' on screen for the life of the page" + ) + assert "withTimeout(" in source, "the timeout must actually be applied to the render" + assert "MAX_SOURCE_LENGTH" in source, ( + "matches INLINE_DIAGRAM_MAX_SOURCE_LENGTH in chat-inline-diagrams.js" + ) + assert "maxTextSize: MERMAID_MAX_TEXT_SIZE" in source, "mermaid's ceilings must be stated" + assert "maxEdges: MERMAID_MAX_EDGES" in source, "mermaid's ceilings must be stated" + print(" ok rendering is bounded by a timeout and by size limits") + + +def test_the_repair_only_runs_after_a_failure(): + """A diagram mermaid accepts is handed over untouched.""" + source = _read(MERMAID_TSX) + + first = source.index("svg = await draw(source);") + repair = source.index("repairMermaidSource(source)") + assert first < repair, ( + "the original source must be attempted before any rewrite, so a diagram that renders " + "today can never be changed by the repair pass" + ) + assert "isRepairWorthTrying(source)" in source, ( + "a second render must be skipped when the repair would change nothing" + ) + assert "catch (error)" in source[first:repair], "the retry must be reached only on failure" + print(" ok source is only repaired after mermaid has already refused it") + + +def test_labels_wrap_at_a_readable_width(): + """Mermaid's 200px default turns long labels into unreadable columns of text.""" + source = _read(MERMAID_TSX) + + assert "MERMAID_WRAPPING_WIDTH" in source, "the wrapping width must be set explicitly" + assert "wrappingWidth: MERMAID_WRAPPING_WIDTH" in source, ( + "the wrapping width must actually be passed to the flowchart renderer" + ) + match = re.search(r"MERMAID_WRAPPING_WIDTH\s*=\s*(\d+)", source) + assert match and int(match.group(1)) > 200, ( + "the point of setting it is to be wider than mermaid's default of 200" + ) + print(" ok labels wrap at a width wider than mermaid's default") + + +def test_the_diagram_can_be_enlarged(): + """Zoom, a resize handle and a full-screen viewer.""" + source = _read(MERMAID_TSX) + stage = _read(STAGE_TSX) + + assert "ZoomControls" in source, "zoom controls must exist" + assert "MIN_ZOOM" in stage and "MAX_ZOOM" in stage, "zoom must be bounded" + assert "DiagramLightbox" in source, "a full-screen viewer must exist" + assert 'role="dialog"' in source and 'aria-modal="true"' in source, ( + "the viewer is a dialog and must say so" + ) + assert "'Escape'" in source, "Escape must dismiss the viewer, as it does for the image one" + + assert 'role="slider"' in stage, ( + "the resize handle must be a slider: it has a value, a range and a reset, and a " + "drag-only affordance would be unusable from the keyboard" + ) + assert "aria-valuemin" in stage and "aria-valuemax" in stage, "the handle must expose its range" + assert "'Home'" in stage, "there must be a way back to the automatic height" + assert "ArrowDown" in stage and "ArrowUp" in stage, "the handle must be keyboard operable" + print(" ok a diagram can be zoomed, resized and opened full screen") + + +def test_the_chosen_height_is_kept_on_the_message(): + """A resize is stored beside the colours, and the two do not disturb each other.""" + message = {"id": "m1"} + + apply_visual_style(message, "mermaid", 0, None, "abc123", 400) + stored = read_visual_styles(message)["mermaid"]["0"] + assert stored["height"] == 400, stored + assert "palette" not in stored, ( + "a resize alone must not become a colour override, or a diagram someone merely made " + "bigger would stop following their default palette" + ) + + # Recolouring says nothing about the height, so the height survives. + apply_visual_style( + message, + "mermaid", + 0, + {"palette": "vivid", "background": "theme", "colors": {}}, + "abc123", + ) + stored = read_visual_styles(message)["mermaid"]["0"] + assert stored["height"] == 400, stored + assert stored["palette"] == "vivid", stored + + # Resetting the colours says nothing about the height either. + apply_visual_style(message, "mermaid", 0, None, "abc123") + stored = read_visual_styles(message)["mermaid"]["0"] + assert stored["height"] == 400, stored + assert "palette" not in stored, stored + + # Clearing the height with no colours left removes the entry entirely. + apply_visual_style(message, "mermaid", 0, None, "abc123", None) + assert read_visual_styles(message) == {}, read_visual_styles(message) + assert "visual_styles" not in message.get("metadata", {}), message + print(" ok a chosen height is stored, kept and cleared independently of the colours") + + +def test_a_stored_height_is_bounded_and_validated(): + """The value comes from a drag, so it is clamped rather than trusted.""" + assert validate_block_height(10) == MIN_BLOCK_HEIGHT + assert validate_block_height(99999) == MAX_BLOCK_HEIGHT + assert validate_block_height(300.4) == 300 + assert validate_block_height(None) is None + + # json.loads accepts the bare Infinity and NaN tokens, and round(inf) raises OverflowError, + # which would escape the route's VisualStyleError handling and turn a bad request into a + # 500 with an ERROR-level traceback. + for bad in ("400", True, [400], {"height": 400}, float("nan"), float("inf"), float("-inf")): + try: + validate_block_height(bad) + except VisualStyleError: + continue + except Exception as error: # noqa: BLE001 + raise AssertionError( + f"a height of {bad!r} raised {type(error).__name__}, which the route does not " + "handle; it must raise VisualStyleError so the request fails with a 400" + ) from error + raise AssertionError(f"a height of {bad!r} should have been rejected") + + message = {"id": "m2"} + apply_visual_style(message, "mermaid", 0, None, "abc123", 10_000_000) + assert read_visual_styles(message)["mermaid"]["0"]["height"] == MAX_BLOCK_HEIGHT + print(" ok a stored height is clamped and non-numbers are refused") + + +def test_a_height_is_not_carried_across_a_source_change(): + """A size chosen for different content must not become authoritative for this block.""" + message = {"id": "m4"} + + apply_visual_style(message, "mermaid", 0, None, "aaaa1111", 800) + assert read_visual_styles(message)["mermaid"]["0"]["height"] == 800 + + # The block at index 0 is now different content: an edit or a mask shifted the positions. + # The client already ignores the stored entry, so carrying the height forward would + # resurrect it and re-stamp it with the new fingerprint. + apply_visual_style( + message, + "mermaid", + 0, + {"palette": "vivid", "background": "theme", "colors": {}}, + "bbbb2222", + ) + stored = read_visual_styles(message)["mermaid"]["0"] + assert "height" not in stored, ( + f"a height stored against source aaaa1111 was carried onto bbbb2222: {stored}" + ) + assert stored["source_hash"] == "bbbb2222", stored + + # A matching fingerprint still keeps the height, which is the whole point of storing it. + apply_visual_style(message, "mermaid", 1, None, "cccc3333", 700) + apply_visual_style( + message, + "mermaid", + 1, + {"palette": "calm", "background": "theme", "colors": {}}, + "cccc3333", + ) + assert read_visual_styles(message)["mermaid"]["1"]["height"] == 700 + print(" ok a stored height does not survive a source-hash change") + + +def test_an_absent_height_is_not_a_cleared_height(): + """The route must tell "said nothing" apart from "clear it".""" + route = _read(APP_DIR / "route_backend_chats.py") + + assert "VISUAL_STYLE_HEIGHT_UNSET" in route, "the sentinel must be imported" + assert "data.get('height') if 'height' in data else VISUAL_STYLE_HEIGHT_UNSET" in route, ( + "a body that omits the height must leave the stored one alone; only an explicit null " + "clears it" + ) + + message = {"id": "m3"} + apply_visual_style(message, "mermaid", 0, None, "abc123", 500) + apply_visual_style(message, "mermaid", 0, None, "abc123", UNSET) + assert read_visual_styles(message)["mermaid"]["0"]["height"] == 500 + print(" ok omitting the height keeps it; sending null clears it") + + +def test_a_height_only_entry_does_not_shadow_the_reader_default(): + """The client must read the colour override from the palette, not the entry.""" + source = _read(BLOCK_STYLE_TS) + + assert "readStoredEntry" in source, "reading the entry and reading the override are separate" + assert re.search(r"typeof entry\.palette !== 'string'", source), ( + "an entry carrying only a height is not a colour override; treating it as one would " + "pin a resized diagram to the built-in palette" + ) + assert "readStoredHeight" in source, "the stored height must be read back" + print(" ok a height-only entry still follows the reader's colour default") + + +def test_the_thread_does_not_re_render_on_every_scroll(): + """The scroll position must not re-run every message's markdown.""" + source = _read(MESSAGE_LIST_TSX) + + assert "const MessageBubble = memo(MessageBubbleInner)" in source, ( + "without memoisation every streaming token re-runs the whole markdown pipeline for " + "every message in the thread" + ) + assert "pinnedRef" in source, "the pinned flag must be a ref" + assert "pinnedToBottom" not in source, ( + "holding the pinned flag in state re-rendered the entire thread on every scroll that " + "crossed the threshold" + ) + assert ".scrollIntoView(" not in source, ( + "scrollIntoView also scrolls every scrollable ancestor; the container's own scrollTop " + "is what should move" + ) + assert "ResizeObserver" in source, ( + "a diagram renders asynchronously and grows the thread after the scroll that was meant " + "to land at the bottom, which is why the bottom became unreachable" + ) + assert "useMemo(() => readMaskState(message)" in source, ( + "mask state is walked on every render of every message and must be memoised" + ) + print(" ok scrolling and streaming no longer re-render the whole thread") + + +def test_the_sanitizer_boundary_is_still_a_single_reviewed_file(): + """The expanded viewer must not become a second, unreviewed HTML sink.""" + sinks = [ + path + for path in (V2_SRC / "components").rglob("*.tsx") + if "dangerouslySetInnerHTML" in _read(path) + ] + names = sorted(path.name for path in sinks) + assert names == ["MathBlock.tsx", "MermaidDiagram.tsx"], ( + f"unexpected HTML sink(s): {names}. The full-screen viewer deliberately lives inside " + "MermaidDiagram.tsx so every place diagram markup reaches the DOM stays in one file " + "that test_v2_rich_rendering.py reviews." + ) + + assert "purify.sanitize(" in _read(MERMAID_TSX), "the boundary itself must still be there" + assert "dangerouslySetInnerHTML" not in _read(STAGE_TSX), ( + "DiagramStage owns sizing only; markup must not reach the DOM through it" + ) + print(" ok diagram markup still reaches the DOM in exactly one reviewed file") + + +def test_no_new_browser_dependency_was_introduced(): + """Nothing here may reach the public Internet or add a package.""" + package = _read(REPO_ROOT / "application" / "v2_ui" / "package.json") + for banned in ("mermaid", "dompurify", "react-zoom", "panzoom", "re-resizable"): + assert f'"{banned}"' not in package, f"{banned} must not become an npm dependency" + + for path in (MERMAID_TSX, STAGE_TSX, SOURCE_TS): + text = _read(path) + urls = re.findall(r"https?://[^\s'\"`)]+", text) + unexpected = [ + url + for url in urls + if url not in ("http://www.w3.org/2000/svg", "http://www.w3.org/1999/xlink") + ] + assert not unexpected, f"{path.name} references {unexpected}" + print(" ok no new browser dependency and no remote asset") + + +def test_the_prompt_guidance_warns_about_what_actually_breaks(): + """Guidance covers the failures reproduced against mermaid, not invented ones.""" + from functions_diagram_operations import build_diagram_guidance_message + + guidance = build_diagram_guidance_message() + + assert "reserved words" in guidance, ( + "`end` as a node id is the single most common parse failure in model output" + ) + for word in ("`end`", "`graph`", "`class`", "`style`"): + assert word in guidance, f"{word} is reserved and must be named" + assert "lowercase `end`" in guidance, "`End` and `END` are not accepted by the grammar" + assert "" in guidance, ( + "carrying angle-bracketed placeholders out of pasted text into labels is what produced " + "the reported failures" + ) + assert "short phrase" in guidance, ( + "one node holding a dozen
lines renders as a column of text nobody can read" + ) + print(" ok the guidance warns about the failures that were actually reproduced") + + +def test_the_typescript_logic_checks_pass(): + """Run the bundled behaviour checks, when the front-end toolchain is installed.""" + ui_dir = REPO_ROOT / "application" / "v2_ui" + check = Path(__file__).with_name("test_v2_diagram_viewer_logic.ts") + + assert check.exists(), "the logic check file is missing" + + if not (ui_dir / "node_modules").exists(): + print(" -- skipped the TypeScript checks: run npm install in application/v2_ui") + return + + # The check file lives in functional_tests/, which has no node_modules of its own, so bare + # imports are left for node to resolve at run time from where the bundle is written. + bundle = ui_dir / "node_modules" / ".cache-diagram-viewer-check.mjs" + try: + subprocess.run( + [ + "npx", + "esbuild", + str(check), + "--bundle", + "--platform=node", + "--format=esm", + "--packages=external", + f"--outfile={bundle}", + "--log-level=error", + ], + cwd=str(ui_dir), + check=True, + shell=(sys.platform == "win32"), + ) + result = subprocess.run( + ["node", str(bundle)], + cwd=str(ui_dir), + capture_output=True, + text=True, + shell=(sys.platform == "win32"), + ) + finally: + if bundle.exists(): + bundle.unlink() + + if result.returncode != 0: + print(result.stdout) + print(result.stderr) + raise AssertionError("the TypeScript logic checks failed") + + passed = result.stdout.count(" ok ") + assert passed > 45, f"expected the full check suite, saw {passed} checks" + print(f" ok {passed} TypeScript logic checks passed") + + +TESTS = [ + test_version_is_at_least_the_implementing_release, + test_the_panel_takes_its_width_from_the_diagram, + test_a_long_diagram_cannot_fill_the_thread, + test_the_render_failure_is_reported_rather_than_swallowed, + test_rendering_is_bounded, + test_the_repair_only_runs_after_a_failure, + test_labels_wrap_at_a_readable_width, + test_the_diagram_can_be_enlarged, + test_the_chosen_height_is_kept_on_the_message, + test_a_stored_height_is_bounded_and_validated, + test_a_height_is_not_carried_across_a_source_change, + test_an_absent_height_is_not_a_cleared_height, + test_a_height_only_entry_does_not_shadow_the_reader_default, + test_the_thread_does_not_re_render_on_every_scroll, + test_the_sanitizer_boundary_is_still_a_single_reviewed_file, + test_no_new_browser_dependency_was_introduced, + test_the_prompt_guidance_warns_about_what_actually_breaks, + test_the_typescript_logic_checks_pass, +] + + +def main(): + print("Testing the V2 diagram viewer...\n") + failures = [] + + for test in TESTS: + try: + test() + except Exception as error: # noqa: BLE001 + failures.append((test.__name__, error)) + print(f" FAIL {test.__name__}: {error}") + + print(f"\n{len(TESTS) - len(failures)}/{len(TESTS)} checks passed") + if failures: + import traceback + + for name, error in failures: + print(f"\n--- {name} ---") + traceback.print_exception(type(error), error, error.__traceback__) + return 1 + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/functional_tests/test_v2_diagram_viewer_logic.ts b/functional_tests/test_v2_diagram_viewer_logic.ts new file mode 100644 index 000000000..27122ac34 --- /dev/null +++ b/functional_tests/test_v2_diagram_viewer_logic.ts @@ -0,0 +1,342 @@ +// test_v2_diagram_viewer_logic.ts +// Behavioural checks for the V2 diagram source repair and diagram sizing logic. +// +// Version: 0.261.037 +// Implemented in: 0.261.037 +// +// The V2 interface has no unit test runner, so this follows test_v2_visual_style_logic.ts: +// bundled with the esbuild Vite already brings in, run under node by +// test_v2_diagram_viewer_controls.py, and skipped when the front-end toolchain is not +// installed. +// +// Every "broken source" below was reproduced as a real mermaid 11.17.2 parse failure in +// Chromium, configured exactly as MermaidDiagram.tsx configures it, and every repaired form +// was confirmed to render. What is asserted here is the repair's behaviour, which is what can +// be checked without a browser: +// +// - a source mermaid accepts is never rewritten, because the repair only ever runs after a +// failure and rewriting working output would be a regression with no upside; +// - each reproduced failure is actually changed, and changed in the specific way that made +// it render; +// - nothing outside a flowchart is touched, since every rule is flowchart grammar; +// - the natural size read back out of mermaid's SVG is the size the panel is built from. + +import { + describeMermaidError, + isRepairWorthTrying, + repairMermaidSource, +} from '../application/v2_ui/src/lib/mermaidSource'; +import { + clampStageHeight, + clampZoom, + defaultStageHeight, + DEFAULT_MAX_STAGE_HEIGHT, + MAX_STAGE_HEIGHT, + MAX_ZOOM, + MIN_STAGE_HEIGHT, + MIN_ZOOM, + readDiagramSize, +} from '../application/v2_ui/src/components/chat/DiagramStage'; + +let failures = 0; +function check(name: string, condition: boolean, detail?: unknown) { + if (condition) { + console.log(` ok ${name}`); + } else { + failures += 1; + console.log(`FAIL ${name}`, detail ?? ''); + } +} + +/* ---- sources mermaid already renders must never be rewritten ---- */ + +const RENDERS = { + 'flowchart with
labels': [ + 'flowchart TD', + ' browser["Browser
User IP present in normal HTTP"]', + '', + ' subgraph azure["Azure compliance boundary"]', + ' app["Simple Chat App Service
Sees: user IP, Entra identity"]', + ' end', + '', + ' browser --> app', + ' app -->|"Authorization: Bearer token
Content-Type: application/json"| browser', + ].join('\n'), + 'flowchart with parentheses in a quoted label': [ + 'flowchart TD', + ' a["Logs: App Insights / App Service logs (your compliance boundary)"]', + ' b["App Service"]', + ' a --> b', + ].join('\n'), + 'flowchart with an equals sign in a label': [ + 'flowchart TD', + ' a["market=en-us, set_lang=en, count=10"]', + ' b["B"]', + ' a --> b', + ].join('\n'), + sequenceDiagram: [ + 'sequenceDiagram', + ' participant Browser', + ' participant App', + ' Browser->>App: request', + ' App-->>Browser: response', + ].join('\n'), + 'stateDiagram-v2': ['stateDiagram-v2', ' [*] --> Idle', ' Idle --> [*]: done'].join('\n'), + erDiagram: ['erDiagram', ' USER ||--o{ ORDER : places'].join('\n'), + classDiagram: ['classDiagram', ' class User {', ' +login()', ' }'].join('\n'), +}; + +for (const [name, source] of Object.entries(RENDERS)) { + check(`repair is a no-op for ${name}`, !isRepairWorthTrying(source), { + repaired: repairMermaidSource(source), + }); +} + +/* ---- each reproduced failure is repaired in the way that made it render ---- */ + +const reserved = repairMermaidSource( + 'flowchart TD\n start["Start"]\n end["End"]\n start --> end\n', +); +check('a reserved node id is renamed at its declaration', reserved.includes('end_node["End"]'), reserved); +check('a reserved node id is renamed at its uses too', reserved.includes('start --> end_node'), reserved); +check('renaming a reserved id leaves no bare declaration', !/\bend\s*\[/.test(reserved), reserved); + +for (const word of ['graph', 'class', 'style']) { + const renamed = repairMermaidSource( + `flowchart TD\n ${word}["X"]\n b["B"]\n ${word} --> b\n`, + ); + check(`the reserved id "${word}" is renamed`, renamed.includes(`${word}_node["X"]`), renamed); +} + +check( + 'a word that merely contains a reserved word is left alone', + repairMermaidSource( + 'flowchart TD\n frontend["Front"]\n backend["Back"]\n frontend --> backend\n', + ).includes('frontend["Front"]'), +); + +const miscased = repairMermaidSource( + 'flowchart TD\n subgraph s["S"]\n a["A"]\n End\n b["B"]\n a --> b\n', +); +check('a capitalised End becomes the lowercase terminator', /^\s*end\s*$/m.test(miscased), miscased); + +const unclosed = repairMermaidSource( + 'flowchart TD\n subgraph s["Azure"]\n a["App"]\n b["B"]\n a --> b\n', +); +check( + 'an unclosed subgraph gains its terminator', + unclosed.split('\n').filter((line) => /^\s*end\s*$/.test(line)).length === 1, + unclosed, +); + +const emptyLabel = repairMermaidSource('flowchart TD\n a[""]\n b["B"]\n a --> b\n'); +check('an empty label becomes a space rather than being dropped', emptyLabel.includes('a[" "]'), emptyLabel); + +const bareParens = repairMermaidSource('flowchart TD\n a[App (main)]\n b["B"]\n a --> b\n'); +check('an unquoted label containing parentheses is quoted', bareParens.includes('a["App (main)"]'), bareParens); + +const bareBraces = repairMermaidSource( + 'flowchart TD\n a["A"]\n b["B"]\n a -->|metadata: {}| b\n', +); +check('braces in an edge label are quoted and escaped', bareBraces.includes('|"metadata: {}"|'), bareBraces); + +const innerQuotes = repairMermaidSource( + 'flowchart TD\n a["He said "hello" loudly"]\n b["B"]\n a --> b\n', +); +check( + 'a quote inside a label is escaped rather than ending it early', + innerQuotes.includes('a["He said "hello" loudly"]'), + innerQuotes, +); + +const angles = repairMermaidSource( + 'flowchart TD\n a["Bearer " ]\n b["B"]\n a -->|x| b\n', +); +check('an angle-bracketed placeholder is escaped, not dropped', angles.includes('<random GUID>'), angles); + +const brKept = repairMermaidSource('flowchart TD\n a["One
Two{}"]\n b["B"]\n a --> b\n'); +check('a line break survives escaping around it', brKept.includes('One
Two'), brKept); + +for (const variant of ['
', '
', '
']) { + const normalised = repairMermaidSource( + `flowchart TD\n a["One${variant}Two{}"]\n b["B"]\n a --> b\n`, + ); + check(`"${variant}" is normalised to
`, normalised.includes('One
Two'), normalised); +} + +const runTogether = repairMermaidSource('flowchart TD\n a["A"] b["B"]\n a --> b\n'); +check( + 'two declarations on one line are split', + runTogether.split('\n').filter((line) => /^\s*[\w-]+\["/.test(line)).length === 2, + runTogether, +); + +const dangling = repairMermaidSource('flowchart TD\n a["A"]\n b["B"]\n a --> b\n b -->\n'); +check('a dangling edge is dropped', !/-->\s*$/m.test(dangling), dangling); + +/* ---- a reserved node id must not eat the subgraph terminators ---- */ + +// `end` as a node id and `subgraph` are both common, and they interact: renaming every `end` +// would rewrite the terminators too, moving everything after them inside the group. The result +// still parses, so it would show the wrong structure rather than failing. +const endAndSubgraph = repairMermaidSource( + [ + 'flowchart TD', + ' subgraph grp["Group"]', + ' a["A"]', + ' end', + ' end["Finish"]', + ' a --> end', + ].join('\n'), +); +check( + 'the subgraph terminator survives a reserved-id rename', + endAndSubgraph.split('\n').filter((line) => /^\s*end\s*$/.test(line)).length === 1, + endAndSubgraph, +); +check( + 'the node called end is still renamed alongside it', + endAndSubgraph.includes('end_node["Finish"]') && endAndSubgraph.includes('a --> end_node'), + endAndSubgraph, +); +check( + 'no terminator is appended, because none was missing', + endAndSubgraph.trimEnd().endsWith('a --> end_node'), + endAndSubgraph, +); + +/* ---- a pipe inside a node label is not an edge-label delimiter ---- */ + +const pipeInLabel = repairMermaidSource( + 'flowchart TD\n a["A|B"] --> |"yes"| b["B"]\n c[Bad (label)]\n', +); +check('a pipe inside a quoted label is left alone', pipeInLabel.includes('a["A|B"]'), pipeInLabel); +check('the arrow beside it is not escaped', pipeInLabel.includes('--> |"yes"|'), pipeInLabel); +check( + 'the genuinely broken node on another line is still repaired', + pipeInLabel.includes('c["Bad (label)"]'), + pipeInLabel, +); + +check( + 'an odd number of pipes is left alone rather than paired by guesswork', + repairMermaidSource('flowchart TD\n a["A"] -->|x b["B"]\n').includes('-->|x'), +); + +check( + 'a byte-order mark is stripped', + !repairMermaidSource('\ufeffflowchart TD\n a["A"]\n').includes('\ufeff'), +); +check( + 'a non-breaking space becomes an ordinary one', + !repairMermaidSource('flowchart TD\n\u00a0\u00a0 a["A"]\n').includes('\u00a0'), +); +check( + 'smart double quotes become straight ones', + repairMermaidSource('flowchart TD\n a[\u201cBrowser\u201d]\n').includes('"Browser"'), +); + +/* ---- nothing outside a flowchart is rewritten ---- */ + +const erCardinality = repairMermaidSource('erDiagram\n USER ||--o{ ORDER : places\n'); +check('erDiagram cardinality is untouched', erCardinality.includes('||--o{ ORDER : places'), erCardinality); + +const sequenceKeyword = repairMermaidSource( + 'sequenceDiagram\n participant end\n end->>end: loop\n', +); +check( + 'a sequence diagram is not rewritten with flowchart rules', + !sequenceKeyword.includes('end_node'), + sequenceKeyword, +); + +check( + 'a comment line is left alone', + repairMermaidSource('flowchart TD\n %% end["not a node"]\n a["A"]\n').includes( + '%% end["not a node"]', + ), +); + +/* ---- error descriptions ---- */ + +check( + 'a parse error is reduced to its first line', + describeMermaidError(new Error('Parse error on line 3:\n...caret art...\nExpecting SEMI')) === + 'Parse error on line 3:', +); +check( + 'the edge limit is reworded', + describeMermaidError(new Error('Edge limit exceeded. 500 edges found, but the limit is 500.')) === + 'The diagram has too many connections to draw.', +); +check( + 'the text size limit is reworded', + describeMermaidError(new Error('Maximum text size in diagram exceeded')) === + 'The diagram source is too large to draw.', +); +check( + 'a missing diagram type is reworded', + describeMermaidError(new Error('No diagram type detected matching given configuration')) === + 'The first line does not name a diagram type mermaid recognises.', +); +check('a non-error is still described', describeMermaidError(undefined).length > 0); + +/* ---- natural size, read out of what mermaid actually emits ---- */ + +// The exact shape mermaid 11.17.2 emits with useMaxWidth: no height attribute, a percentage +// width, and the natural width in a max-width declaration. +const emitted = + ''; + +const size = readDiagramSize(emitted); +check('the natural width is read from max-width', size?.width === 1094, size); +check('the natural height is read from the viewBox', size?.height === 541, size); +check( + 'a percentage width is never mistaken for a size', + readDiagramSize('') === null, +); +check('markup with no size at all reports none', readDiagramSize('') === null); + +const viewBoxOnly = readDiagramSize(''); +check('the viewBox alone is enough', viewBoxOnly?.width === 400 && viewBoxOnly?.height === 200, viewBoxOnly); + +/* ---- stage sizing ---- */ + +check('a stage cannot be dragged below the minimum', clampStageHeight(10) === MIN_STAGE_HEIGHT); +check('a stage cannot be dragged above the maximum', clampStageHeight(99999) === MAX_STAGE_HEIGHT); +check('a stage height is rounded to whole pixels', clampStageHeight(300.6) === 301); + +check('zoom is bounded below', clampZoom(0.01) === MIN_ZOOM); +check('zoom is bounded above', clampZoom(100) === MAX_ZOOM); + +// The tree diagram from the report: wide and short. Fitted to a narrower panel it is shorter +// still, so its stage should be no taller than it needs. +const wideAndShort = defaultStageHeight({ width: 1094, height: 541 }, 700); +check( + 'a wide, short diagram gets a stage that fits it', + wideAndShort < DEFAULT_MAX_STAGE_HEIGHT && wideAndShort > MIN_STAGE_HEIGHT, + wideAndShort, +); + +// The label-heavy diagram from the report: narrow and very tall. Its stage must be capped, or +// it becomes a thousand-pixel block in the middle of the thread. +check( + 'a tall diagram is capped rather than filling the thread', + defaultStageHeight({ width: 497, height: 867 }, 497) === DEFAULT_MAX_STAGE_HEIGHT, + defaultStageHeight({ width: 497, height: 867 }, 497), +); + +// 500 edges measured 50,466px tall. Nothing that size may ever reach the scroll container. +check( + 'an enormous diagram is capped', + defaultStageHeight({ width: 111, height: 50466 }, 800) === DEFAULT_MAX_STAGE_HEIGHT, +); + +check( + 'an unmeasured diagram still gets a usable stage', + defaultStageHeight(null, 800) === MIN_STAGE_HEIGHT, +); + +console.log(failures === 0 ? '\nAll checks passed.' : `\n${failures} check(s) failed.`); +process.exit(failures === 0 ? 0 : 1);