Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions desktop/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions desktop/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
"@codemirror/theme-one-dark": "^6.1.3",
"@codemirror/view": "^6.43.1",
"@excalidraw/excalidraw": "^0.18.1",
"@excalidraw/mermaid-to-excalidraw": "^2.2.2",
"@milkdown/kit": "^7.21.2",
"@milkdown/react": "^7.21.2",
"@milkdown/theme-nord": "^7.21.2",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ vi.mock("@excalidraw/excalidraw", () => ({
),
}));
vi.mock("@excalidraw/excalidraw/index.css", () => ({}));
// Keep the heavy mermaid parser out of jsdom; no test element is a diagram.
vi.mock("@excalidraw/mermaid-to-excalidraw", () => ({
parseMermaidToExcalidraw: vi.fn(async () => ({ elements: [], files: {} })),
}));

import { ExcalidrawBoard } from "../canvas/ExcalidrawBoard";
import type { CanvasElement } from "../canvas/canvas-api";
Expand Down
29 changes: 29 additions & 0 deletions desktop/src/apps/ProjectsApp/__tests__/mermaid-to-elements.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { describe, it, expect, vi } from "vitest";

vi.mock("@excalidraw/mermaid-to-excalidraw", () => ({
parseMermaidToExcalidraw: vi.fn(async (src: string) => {
if (src.includes("BOOM")) throw new Error("invalid mermaid");
return { elements: [{ type: "rectangle", x: 5, y: 7, width: 10, height: 10 }], files: {} };
}),
}));
vi.mock("@excalidraw/excalidraw", () => ({
convertToExcalidrawElements: (els: unknown[]) => els,
}));

import { mermaidToExcalidraw } from "../canvas/mermaid-to-elements";

describe("mermaidToExcalidraw", () => {
it("converts the diagram and translates it by the element offset", async () => {
const out = await mermaidToExcalidraw("graph TD\n A-->B", 100, 200);
expect(out).toHaveLength(1);
expect(out[0]).toMatchObject({ type: "rectangle", x: 105, y: 207 });
});

it("returns an empty array for a blank source", async () => {
expect(await mermaidToExcalidraw(" ", 0, 0)).toEqual([]);
});

it("returns an empty array when the mermaid source is invalid", async () => {
expect(await mermaidToExcalidraw("BOOM", 0, 0)).toEqual([]);
});
});
93 changes: 78 additions & 15 deletions desktop/src/apps/ProjectsApp/canvas/ExcalidrawBoard.tsx
Original file line number Diff line number Diff line change
@@ -1,43 +1,106 @@
import { useMemo, useState, useEffect } from "react";
import { useMemo, useState, useEffect, useRef } from "react";
import type { ComponentProps } from "react";
import { Excalidraw, convertToExcalidrawElements } from "@excalidraw/excalidraw";
import "@excalidraw/excalidraw/index.css";
import { CanvasElement } from "./canvas-api";
import { elementsToSkeletons } from "./element-to-excalidraw";
import { elementToSkeleton } from "./element-to-excalidraw";
import { mermaidToExcalidraw, type ExcalidrawElements } from "./mermaid-to-elements";

// Read-only Excalidraw view over the canonical CanvasElement scene. Slice 2 of
// the tldraw -> Excalidraw migration: the board is built alongside the existing
// tldraw board and not yet wired into CanvasView. Write-back interactions,
// diagram rendering, and the swap that retires tldraw are later slices.
// Read-only Excalidraw view over the canonical CanvasElement scene (tldraw ->
// Excalidraw migration). Most kinds map synchronously; mermaid/flowchart kinds
// render their real diagram via mermaid-to-excalidraw, falling back to the
// placeholder rectangle while the (async) conversion runs or if it fails. The
// board is not yet wired into CanvasView; write-back interactions and the swap
// that retires tldraw are later slices.

const DIAGRAM_KINDS = new Set(["mermaid", "flowchart"]);

type ExcalidrawAPI = Parameters<
NonNullable<ComponentProps<typeof Excalidraw>["excalidrawAPI"]>
>[0];

type SkeletonInput = Parameters<typeof convertToExcalidrawElements>[0];

export interface ExcalidrawBoardProps {
elements: CanvasElement[];
theme?: "light" | "dark";
}

function live(elements: CanvasElement[]): CanvasElement[] {
return elements
.filter((el) => el.deleted_at == null)
.slice()
.sort((a, b) => (a.z_index || 0) - (b.z_index || 0));
}

export function ExcalidrawBoard({ elements, theme = "light" }: ExcalidrawBoardProps) {
const [api, setApi] = useState<ExcalidrawAPI | null>(null);
// Converted diagram elements keyed by the CanvasElement id; absent until the
// async mermaid conversion resolves.
const [diagrams, setDiagrams] = useState<Record<string, ExcalidrawElements>>({});

const sceneElements = useMemo(
// Re-run conversion only when a diagram element's id/source/position changes.
const diagramKey = useMemo(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WARNING: diagramKey invalidates on every unrelated element change, re-converting all diagrams.

The useMemo for diagramKey lists elements as its only dependency. elements is a fresh array reference whenever the scene changes (e.g., editing a note's text payload, dragging a sticky), so the memo recomputes JSON.stringify(...) and returns a new string even when the diagram-relevant inputs (id, x, y, source) are unchanged. A new diagramKey triggers the conversion effect, so adding/removing/editing any non-diagram element re-parses every mermaid source on the board.

Either drop the memoization and depend on a derived signature (e.g., a useMemo that returns the diagram list itself with elements as the dep), or hash only the relevant fields with a stable key.


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

() =>
convertToExcalidrawElements(
elementsToSkeletons(elements) as unknown as Parameters<
typeof convertToExcalidrawElements
>[0],
JSON.stringify(
live(elements)
.filter((el) => DIAGRAM_KINDS.has(el.kind))
.map((el) => [el.id, el.x, el.y, (el.payload || {}).source]),
),
[elements],
);

// Excalidraw opens at scroll origin, so a scene whose elements sit away from
// (0,0) renders off-screen. Fit the viewport to the content once the API is
// ready and whenever the scene changes.
useEffect(() => {
if (api && sceneElements.length > 0) {
let cancelled = false;
const diagramEls = live(elements).filter((el) => DIAGRAM_KINDS.has(el.kind));
if (diagramEls.length === 0) {
setDiagrams({});
return;
}
(async () => {
// Convert diagrams in parallel: each parse is independent, so the total
// wait is the slowest single diagram, not the sum.
const converted = await Promise.all(
diagramEls.map(async (el) => {
const src = String((el.payload || {}).source ?? "");
return [el.id, await mermaidToExcalidraw(src, el.x, el.y)] as const;
}),
);
if (!cancelled) setDiagrams(Object.fromEntries(converted));
})();
Comment on lines +60 to +70

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Quality: Sequential awaits make multi-diagram conversion slow

The conversion loop awaits mermaidToExcalidraw for each diagram element serially inside a for loop, so total latency is the sum of all conversions. mermaid parsing is comparatively heavy; with several diagrams on a board the placeholder rectangles will persist noticeably longer than necessary. Each conversion is independent, so Promise.all(diagramEls.map(...)) would parallelize them and shorten time-to-render. If you prefer incremental display, set state per-element as each resolves instead of building one next map at the end.

Was this helpful? React with 👍 / 👎

return () => {
cancelled = true;
};
// diagramKey captures the inputs that affect conversion.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [diagramKey]);

const sceneElements = useMemo(() => {
// Walk the elements in z_index order (live() sorts them) so cross-kind
// layering is preserved: a diagram is not forced above a regular element
// that has a higher z_index. A ready diagram contributes its converted
// elements; everything else (regular kinds, and a diagram still converting)
// maps through the base skeleton.
return live(elements).flatMap((el) => {
if (DIAGRAM_KINDS.has(el.kind)) {
const ready = diagrams[el.id];
if (ready && ready.length > 0) return ready;
}
return convertToExcalidrawElements([elementToSkeleton(el)] as unknown as SkeletonInput);
});
}, [elements, diagrams]);
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// Excalidraw reads initialData once at mount; push later scenes (async
// diagrams) through the imperative API. Fit the viewport to the content only
// on the first non-empty scene -- refitting on every update would yank a
// user's pan/zoom back once interactions land.
const hasFit = useRef(false);
useEffect(() => {
if (!api) return;
api.updateScene({ elements: sceneElements });
if (!hasFit.current && sceneElements.length > 0) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SUGGESTION: Fit-once can lock the viewport to the placeholder extent

On mount, sceneElements is built from skeletons + placeholder rectangles for diagrams that are still being converted asynchronously. If any element exists, this effect runs scrollToContent against the placeholder scene and flips hasFit.current to true. When the async conversion later resolves and replaces placeholders with the real (often larger) mermaid diagram, hasFit.current is now true, so the viewport never re-fits to the larger content. The user sees a zoom/pan centered on the placeholder rectangle while the actual diagram extends beyond the viewport.

Consider fitting on the first non-empty scene after diagrams have resolved (e.g. gate on diagrams being populated, or re-fit whenever diagrams transitions from empty → non-empty even after the initial fit).


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

api.scrollToContent(sceneElements, { fitToContent: true, animate: false });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SUGGESTION: scrollToContent fires on every scene update, including async diagram completions.

The effect re-runs whenever sceneElements changes (initial mount, regular edits, and after each batch of async diagram conversions finishes). Calling scrollToContent(... { animate: false }) every time will jump the viewport to fit content each time a diagram finishes parsing, which is jarring when the user is panning/zoomed in. Consider only fitting on initial mount (when the API first becomes available) or when the content bounding box grows/shrinks meaningfully, not on every updateScene.


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

hasFit.current = true;
}
}, [api, sceneElements]);
Comment on lines +98 to 105

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Performance: Viewport refits on every scene update, resetting pan/zoom

The effect calls api.updateScene(...) and then api.scrollToContent(..., { fitToContent: true }) every time sceneElements changes. Because diagrams resolve asynchronously, each diagram conversion (and any change to the elements prop) re-fits the viewport, snapping the view back to fit-content even if the user has panned/zoomed. With multiple diagrams resolving one-by-one (see sequential loop), the board will visibly jump several times on load.

Consider running scrollToContent only once (e.g. the first time api becomes available and the scene is non-empty), and using a ref/flag to avoid re-fitting on subsequent async scene pushes. While the board is read-only today, the refit-on-every-update behavior will be disruptive once interactions land in later slices.

Was this helpful? React with 👍 / 👎


Expand Down
32 changes: 32 additions & 0 deletions desktop/src/apps/ProjectsApp/canvas/mermaid-to-elements.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { parseMermaidToExcalidraw } from "@excalidraw/mermaid-to-excalidraw";
import { convertToExcalidrawElements } from "@excalidraw/excalidraw";

// Convert a mermaid/flowchart source string into positioned Excalidraw elements
// for the canvas board. parseMermaidToExcalidraw yields skeleton elements laid
// out from the diagram's own origin; convertToExcalidrawElements fills them in,
// then we translate the whole diagram to the CanvasElement's (x, y). Returns an
// empty array on invalid mermaid so a bad source degrades to nothing (the board
// keeps the placeholder) rather than throwing.

export type ExcalidrawElements = ReturnType<typeof convertToExcalidrawElements>;

export async function mermaidToExcalidraw(
source: string,
offsetX: number,
offsetY: number,
): Promise<ExcalidrawElements> {
const src = (source || "").trim();
if (!src) return [];
try {
const { elements } = await parseMermaidToExcalidraw(src);
const converted = convertToExcalidrawElements(elements);
// Translate the diagram as a whole; bindings between nodes are relative, so
// shifting every element by the same offset preserves them.
return converted.map((el) => ({ ...el, x: el.x + offsetX, y: el.y + offsetY }));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WARNING: Translating only x/y may leave arrow geometry misaligned.

The comment claims "shifting every element by the same offset preserves them" because bindings are relative. That is true for startBinding/endBinding (which Excalidraw resolves against referenced elements' current positions), but convertToExcalidrawElements may also produce elements with points arrays that are absolute coordinates relative to the canvas origin for some shape types. Shifting only x/y while leaving points unchanged will produce visual drift on those elements. Verify against the real @excalidraw/mermaid-to-excalidraw output (e.g., arrows and lines) and translate any positional arrays the library emits.


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

} catch (err) {
// Degrading to the placeholder is the intended UX, but a swallowed parse
// error gives no signal in dev; log it so a malformed source is debuggable.
console.warn("mermaid conversion failed; showing placeholder", err);
return [];
}
}
Loading