Repository navigation
feat(ui): render Mermaid diagrams locally - #898
dajiaohuang wants to merge 14 commits into
Conversation
Assisted-by: gpt-6.1-terra
[Medium impact] The PR changes how images and diagrams appear across several UI reading views. The PR appears safe to merge, with three non-blocking issues in diagram image resolution. FindingsFix with agent prompt### Issue 1
ui/src/mermaidImages.ts:18-19
`resolveMermaidImages` turns `file:///tmp/chart.svg#plot` into the raw file URL without `#plot`. SVG images can therefore show the wrong view, and local links lose their selected section. Ordinary Markdown images preserve this through `chatImageTarget`. Preserve the fragment when building the diagram URL too.
### Issue 2
ui/src/mermaidImages.ts:19
`resolveMermaidImages` uses only the first image candidate. In chat, `artifacts/chart.png` can exist in the session checkout rather than the artifact store; `chatImageTargets` supplies fallback locations for that case. The diagram requests only the artifact-store URL, leaving a broken image when a later candidate exists. Try the remaining candidates when the first image cannot load.
### Issue 3
ui/src/components/ArtifactsTab.tsx:154-156
Artifact images inside diagrams do not refresh when the image file changes. `mermaidImages` returns an unversioned URL and stays unchanged when `entries` changes. With unchanged Markdown source, `MermaidBlock` keeps the same SVG and image URL. Ordinary artifact images already include the image entry's version. Include that version here and update the rendered image URLs when it changes.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryAdds local Mermaid rendering for Markdown fences and standalone diagram files.
No runtime tests or browser captures were performed during this review.
|
| const native = nativeMarkdownPath(src); | ||
| const url = native ? absoluteFileUrl(native) : resolve?.(src)[0]?.url; |
There was a problem hiding this comment.
Local destinations lose fragments
resolveMermaidImages turns file:///tmp/chart.svg#plot into the raw file URL without #plot. SVG images can therefore show the wrong view, and local links lose their selected section. Ordinary Markdown images preserve this through chatImageTarget. Preserve the fragment when building the diagram URL too.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/mermaidImages.ts
Line: 18-19
Comment:
**Local destinations lose fragments**
`resolveMermaidImages` turns `file:///tmp/chart.svg#plot` into the raw file URL without `#plot`. SVG images can therefore show the wrong view, and local links lose their selected section. Ordinary Markdown images preserve this through `chatImageTarget`. Preserve the fragment when building the diagram URL too.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| } | ||
| if (isExternalMarkdownTarget(src) && !isWindowsDrivePath(src) && !nativeMarkdownPath(src)) continue; | ||
| const native = nativeMarkdownPath(src); | ||
| const url = native ? absoluteFileUrl(native) : resolve?.(src)[0]?.url; |
There was a problem hiding this comment.
resolveMermaidImages uses only the first image candidate. In chat, artifacts/chart.png can exist in the session checkout rather than the artifact store; chatImageTargets supplies fallback locations for that case. The diagram requests only the artifact-store URL, leaving a broken image when a later candidate exists. Try the remaining candidates when the first image cannot load.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/mermaidImages.ts
Line: 19
Comment:
**Available images do not load**
`resolveMermaidImages` uses only the first image candidate. In chat, `artifacts/chart.png` can exist in the session checkout rather than the artifact store; `chatImageTargets` supplies fallback locations for that case. The diagram requests only the artifact-store URL, leaving a broken image when a later candidate exists. Try the remaining candidates when the first image cannot load.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| const target = artifactTarget(projectId, folder, src, artifactUrl); | ||
| return target ? [{ url: target.url, resolveAsset: () => null }] : []; | ||
| }, [absoluteFileUrl, artifactUrl, projectId, folder]); |
There was a problem hiding this comment.
Artifact images inside diagrams do not refresh when the image file changes. mermaidImages returns an unversioned URL and stays unchanged when entries changes. With unchanged Markdown source, MermaidBlock keeps the same SVG and image URL. Ordinary artifact images already include the image entry's version. Include that version here and update the rendered image URLs when it changes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/components/ArtifactsTab.tsx
Line: 154-156
Comment:
**Changed charts stay stale**
Artifact images inside diagrams do not refresh when the image file changes. `mermaidImages` returns an unversioned URL and stays unchanged when `entries` changes. With unchanged Markdown source, `MermaidBlock` keeps the same SVG and image URL. Ordinary artifact images already include the image entry's version. Include that version here and update the rendered image URLs when it changes.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Assisted-by: gpt-6.1-terra
Assisted-by: gpt-6.1-terra
Assisted-by: gpt-6.1-terra
Assisted-by: gpt-6.1-terra
Assisted-by: gpt-6.1-terra
af88a58 to
1a12218
Compare
Assisted-by: gpt-6.1-terra
Assisted-by: gpt-6.1-terra
Assisted-by: gpt-6.1-terra
Assisted-by: gpt-6.1-terra
|
thanks for sticking with this one. pele was the right pick and the core works: we ran the patch locally and every diagram family rendered in light and dark, math and local images worked, external images stayed blocked, file links opened in a tab, and the checks pass. it doesn't look good enough to merge though. we need the diagrams to look and feel good in the app, and that part needs a human to actually look at the output and make the taste calls. another agent pass won't get there. Clover doesn't have the time to make those calls for you, so until someone has gone over it by eye, this stays a draft. share screenshots on the next revision. blocking, all of these:
|
Assisted-by: gpt-6.1-terra
Assisted-by: gpt-6.1-terra
Closes #430
Mermaid fences and standalone diagram files can be rendered locally with Pele, including local images and math. Invalid or oversized diagrams fall back to source. Rendering runs in a worker, and diagram links use the existing file navigation where available.
This draft addresses the mechanical review feedback: standalone artifact diagrams, a loading placeholder instead of the timed source flash, a compact source control, chart theme tokens, shared native-path and anchor handling, and normalized Windows paths. The separate XML parser optimization has been removed. The app-wide media changes were split out; Markdown network images now follow the shared behavior merged in #909. Pele is needed to render the supported Mermaid families locally without contacting a network service.
Human visual review and screenshots for the revised output are still outstanding; this remains a draft. The loading placeholder has not been proven to eliminate all layout shift. Diagram-family coverage, accessibility, real application flows, performance, and non-Windows behavior remain incomplete. The current automated browser captures are synthetic fixtures rather than visual acceptance.
At
a00b8b0on Windows, direct TypeScript checking passed; 35 focused Mermaid and Markdown target tests passed; a Chrome fixture passed for chat and artifact Markdown links without remote image requests.git diff --checkpassed. These checks do not cover the full UI suite, all app surfaces, or other platforms. Earlier performance and full-suite observations apply to previous heads and do not validate this revision.Assisted-by: gpt-6.1-terra