feat(canvas): render real mermaid/flowchart diagrams (slice 4)#1411
Conversation
…w board (slice 4) Adds @excalidraw/mermaid-to-excalidraw and mermaid-to-elements.ts: convert a mermaid/flowchart CanvasElement's source to positioned Excalidraw elements (parseMermaidToExcalidraw -> convertToExcalidrawElements -> translate to the element's x/y). ExcalidrawBoard runs the conversion in an effect and pushes the result through the imperative API, falling back to the placeholder rectangle while the async conversion runs or if the source is invalid. Verified visually: a 'graph TD' source renders as a real flowchart (Start -> Decide diamond -> yes/Ship, no/Iterate) beside a sticky note. Unit tests cover the converter (translate + empty/invalid -> []); board smoke tests still pass; tsc clean; 64 ProjectsApp tests green.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
More reviews will be available in 25 minutes and 37 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds ChangesMermaid diagram rendering in Excalidraw canvas
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| // Fallback: the placeholder rectangle from the base mapping. | ||
| return convertToExcalidrawElements([elementToSkeleton(el)] as unknown as SkeletonInput); | ||
| }); | ||
| return [...regularScene, ...diagramScene]; |
There was a problem hiding this comment.
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.
|
|
||
| const sceneElements = useMemo( | ||
| // Re-run conversion only when a diagram element's id/source/position changes. | ||
| const diagramKey = useMemo( |
There was a problem hiding this comment.
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.
| if (!api) return; | ||
| api.updateScene({ elements: sceneElements }); | ||
| if (sceneElements.length > 0) { | ||
| api.scrollToContent(sceneElements, { fitToContent: true, animate: false }); |
There was a problem hiding this comment.
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.
| } | ||
| (async () => { | ||
| const next: Record<string, ExcalidrawElements> = {}; | ||
| for (const el of diagramEls) { |
There was a problem hiding this comment.
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 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 })); |
There was a problem hiding this comment.
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.
Code Review SummaryStatus: No New Issues Found | Recommendation: Approve (incremental) Overview
The incremental diff ( Verification of changed linesLines 78-91 ( Line 6 — drops the now-unused No new issues introduced on the changed lines. Resolved since previous review
Outstanding from previous review (on unchanged lines — out of scope for incremental review)
Files Reviewed (1 file)
Previous Review Summaries (2 snapshots, latest commit f7727ef)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit f7727ef)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Resolved since previous review
Outstanding from previous review (still on changed lines)
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 5af005a)Status: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (5 files)
Reviewed by minimax-m3 · Input: 43.3K · Output: 3.4K · Cached: 107.7K |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| useEffect(() => { | ||
| if (!api) return; | ||
| api.updateScene({ elements: sceneElements }); | ||
| if (sceneElements.length > 0) { | ||
| api.scrollToContent(sceneElements, { fitToContent: true, animate: false }); | ||
| } | ||
| }, [api, sceneElements]); |
There was a problem hiding this comment.
💡 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 👍 / 👎
| (async () => { | ||
| const next: Record<string, ExcalidrawElements> = {}; | ||
| for (const el of diagramEls) { | ||
| const src = String((el.payload || {}).source ?? ""); | ||
| next[el.id] = await mermaidToExcalidraw(src, el.x, el.y); | ||
| } | ||
| if (!cancelled) setDiagrams(next); | ||
| })(); |
There was a problem hiding this comment.
💡 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 👍 / 👎
| } catch { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
💡 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 👍 / 👎
|
Note Your trial team has used its Gitar budget, so automatic reviews are paused. Upgrade now to unlock full capacity. Comment "Gitar review" to trigger a review manually. Code Review 👍 Approved with suggestions 0 resolved / 3 findingsIntegrates real mermaid diagram rendering by converting sources into Excalidraw elements. Address the viewport reset bug on scene updates, optimize conversion latency by parallelizing awaits, and improve error visibility by surfacing invalid mermaid syntax. 💡 Performance: Viewport refits on every scene update, resetting pan/zoom📄 desktop/src/apps/ProjectsApp/canvas/ExcalidrawBoard.tsx:94-100 The effect calls Consider running 💡 Quality: Sequential awaits make multi-diagram conversion slow📄 desktop/src/apps/ProjectsApp/canvas/ExcalidrawBoard.tsx:60-67 The conversion loop awaits 💡 Quality: Invalid mermaid is swallowed silently with no diagnostics📄 desktop/src/apps/ProjectsApp/canvas/mermaid-to-elements.ts:26-28
🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change:
Important Your trial ends in 3 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
desktop/src/apps/ProjectsApp/canvas/ExcalidrawBoard.tsx (1)
60-65: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRun diagram conversions in parallel to reduce placeholder latency.
Lines 62–65 process each diagram serially. For multiple diagrams, total wait time becomes the sum of all parse times.
⚡ Proposed refactor
(async () => { - const next: Record<string, ExcalidrawElements> = {}; - for (const el of diagramEls) { - const src = String((el.payload || {}).source ?? ""); - next[el.id] = await mermaidToExcalidraw(src, el.x, el.y); - } + const entries = await Promise.all( + diagramEls.map(async (el) => { + const src = String((el.payload || {}).source ?? ""); + const converted = await mermaidToExcalidraw(src, el.x, el.y); + return [el.id, converted] as const; + }), + ); + const next: Record<string, ExcalidrawElements> = Object.fromEntries(entries); if (!cancelled) setDiagrams(next); })();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src/apps/ProjectsApp/canvas/ExcalidrawBoard.tsx` around lines 60 - 65, The for loop iterating over diagramEls currently processes each diagram conversion sequentially with await, causing total latency to be the sum of all conversions. Replace the sequential for loop with a parallel approach by using Promise.all() to create all mermaidToExcalidraw promises simultaneously, then await them all at once. Map each el from diagramEls to a mermaidToExcalidraw call, resolve all promises in parallel, and then populate the next object with the results in a single operation to significantly reduce the total wait time for multiple diagrams.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@desktop/src/apps/ProjectsApp/canvas/ExcalidrawBoard.tsx`:
- Around line 75-90: The current implementation in the sceneElements useMemo
hook separates elements by kind (regular vs diagram) into separate arrays and
then concatenates them, which loses the original z-index ordering. To preserve
cross-kind z-order, iterate through the original elements array in its original
order, and for each element, check if it is a diagram kind using DIAGRAM_KINDS
and convert it appropriately (using the diagram from the diagrams mapping if
available, or the elementToSkeleton fallback). This way, elements maintain their
intended stacking order regardless of their kind. The result should interleave
regularScene and diagramScene elements based on the original elements array
ordering instead of concatenating all regular elements first followed by all
diagram elements.
---
Nitpick comments:
In `@desktop/src/apps/ProjectsApp/canvas/ExcalidrawBoard.tsx`:
- Around line 60-65: The for loop iterating over diagramEls currently processes
each diagram conversion sequentially with await, causing total latency to be the
sum of all conversions. Replace the sequential for loop with a parallel approach
by using Promise.all() to create all mermaidToExcalidraw promises
simultaneously, then await them all at once. Map each el from diagramEls to a
mermaidToExcalidraw call, resolve all promises in parallel, and then populate
the next object with the results in a single operation to significantly reduce
the total wait time for multiple diagrams.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d1e95a73-7b0e-4dff-ba77-4d36170ff033
⛔ Files ignored due to path filters (1)
desktop/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
desktop/package.jsondesktop/src/apps/ProjectsApp/__tests__/ExcalidrawBoard.test.tsxdesktop/src/apps/ProjectsApp/__tests__/mermaid-to-elements.test.tsdesktop/src/apps/ProjectsApp/canvas/ExcalidrawBoard.tsxdesktop/src/apps/ProjectsApp/canvas/mermaid-to-elements.ts
…mermaid errors - Convert mermaid/flowchart diagrams in parallel (Promise.all) so total latency is the slowest single diagram, not the sum. - Fit the viewport to content only on the first non-empty scene, so a later scene update (async diagrams, and write-back once interactions land) no longer resets the user's pan/zoom. - Log a swallowed mermaid parse error instead of discarding it silently, while still degrading to the placeholder. Re-verified the flowchart still renders after these changes.
| useEffect(() => { | ||
| if (!api) return; | ||
| api.updateScene({ elements: sceneElements }); | ||
| if (!hasFit.current && sceneElements.length > 0) { |
There was a problem hiding this comment.
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.
Composing the scene as [all regular, then all diagrams] forced every diagram above every regular element regardless of z_index. Walk the elements in z_index order instead (live() already sorts them) and map each in place, so a diagram with a low z_index can sit below a regular element with a higher one.
1 participant
Slice 4 of the tldraw to Excalidraw migration: mermaid/flowchart CanvasElements now render as real diagrams instead of placeholder rectangles.
What
mermaid-to-elements.ts:mermaidToExcalidraw(source, x, y)converts a mermaid source via@excalidraw/mermaid-to-excalidrawthenconvertToExcalidrawElements, translating the diagram to the element's position. Returns[]on blank/invalid source.ExcalidrawBoard: an effect converts each mermaid/flowchart element asynchronously and pushes the scene through the imperative API; the placeholder rectangle shows while converting or on failure.Verified
Screenshotted locally: a
graph TDsource renders as a flowchart (Start -> Decide diamond -> yes/Ship, no/Iterate) next to a sticky note, in Excalidraw's hand-drawn style. No console errors.Tests
Converter unit tests (translate offset, empty/invalid -> []); board smoke tests; tsc clean; 64 ProjectsApp tests pass.
Remaining canvas slices
3 write-back interactions, 5 swap behind a flag (live verify), 6 remove tldraw.
Summary by CodeRabbit