Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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([]);
});
});
86 changes: 72 additions & 14 deletions desktop/src/apps/ProjectsApp/canvas/ExcalidrawBoard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,40 +3,98 @@ 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 { elementsToSkeletons, 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 () => {
const next: Record<string, ExcalidrawElements> = {};
for (const el of diagramEls) {

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: Diagram conversions run serially with for...of await.

When N mermaid elements exist on the board, conversion takes ~Nx the per-diagram parse latency. parseMermaidToExcalidraw is independent per source, so await Promise.all(diagramEls.map(...)) (or batches) would let diagrams render in roughly the slowest single parse time instead of the sum.


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

const src = String((el.payload || {}).source ?? "");
next[el.id] = await mermaidToExcalidraw(src, el.x, el.y);
}
if (!cancelled) setDiagrams(next);
})();
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(() => {
const els = live(elements);
const regular = els.filter((el) => !DIAGRAM_KINDS.has(el.kind));
const regularScene = convertToExcalidrawElements(
elementsToSkeletons(regular) as unknown as SkeletonInput,
);
const diagramScene = els
.filter((el) => DIAGRAM_KINDS.has(el.kind))
.flatMap((el) => {
const ready = diagrams[el.id];
if (ready && ready.length > 0) return ready;
// Fallback: the placeholder rectangle from the base mapping.
return convertToExcalidrawElements([elementToSkeleton(el)] as unknown as SkeletonInput);
});
return [...regularScene, ...diagramScene];

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: Z-ordering bug — regular elements always render below all diagram elements.

els is sorted by z_index ascending, but then you split it into regular (preserves relative z-order among regulars) and diagramScene (preserves relative z-order among diagrams), then concatenate [...regularScene, ...diagramScene]. Excalidraw draws in array order, so every diagram paints on top of every regular element regardless of z_index. A diagram with z_index: 1 will render over a note with z_index: 1000.

Merge the two arrays and re-sort by z_index before passing to convertToExcalidrawElements, or build a single skeleton list interleaved by z_index and pass the whole thing through one convertToExcalidrawElements call.


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

}, [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.
useEffect(() => {
if (!api) return;
api.updateScene({ elements: sceneElements });
if (sceneElements.length > 0) {
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.

}
}, [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
29 changes: 29 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,29 @@
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 {
return [];
}

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: Invalid mermaid is swallowed silently with no diagnostics

catch { return []; } discards the parse error entirely. Degrading to the placeholder rectangle is the intended UX, but with no console.warn/debug logging a malformed mermaid source becomes invisible to developers and users alike — the diagram just silently stays a placeholder with no indication why. Consider logging the caught error (at least in dev) so authoring mistakes in the source can be diagnosed.

Was this helpful? React with 👍 / 👎

}
Loading