Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mermaid duplicating ids #1440

Closed
williamthome opened this issue Sep 25, 2022 · 11 comments
Closed

Mermaid duplicating ids #1440

williamthome opened this issue Sep 25, 2022 · 11 comments
Labels
bug Something isn't working upstream The issue must be tackled upstream

Comments

@williamthome
Copy link

Environment

  • Elixir & Erlang/OTP versions (elixir --version): elixir 1.14.0-rc.1-otp-25 | erlang 25.0.4
  • Operating system: Ubuntu 22.04.1 LTS
  • How have you started Livebook (mix phx.server, livebook CLI, Docker, etc): livebook server start.livemd
  • Livebook version (use git rev-parse HEAD if running with mix): 3980e48ec9dc262655fac8bbbafdd2a3b036dbfd
  • Browsers that reproduce this bug (the more the merrier): Google Chrome Version 105.0.5195.125 (Official Build) (64-bit)
  • Include what is logged in the browser console: Multiple IDs detected: flowchart-pointEnd. Ensure unique element ids.
  • Include what is logged to the server console: [Livebook] Application running at http://localhost:8080/?token=44td7pnpymvtxq3selnurxs3s4cw6iyo

Current behavior

I just cloned the DockYard-Academy/beta_curriculum and I'm receiving a bunch of errors of duplicated ids. See the gif below (sorry for the intro delay, but it took some time to raise the errors):
livebook_mermaid_issue
The error occurs just by entering the on the page Week 1: Core Syntax | 1. Course Tools or clicking on some evaluate block.

Investigating, I found the error comes from mermaid.
The referred page contains these MD blocks:

mermaid
flowchart LR
top --> folder_1 --> folder_2 --> folder_3
top --> folder_a --> folder_b --> folder_c
style top fill:lightgreen

flowchart LR
top --> folder_1 --> folder_2 --> folder_3
top --> folder_a --> folder_b --> folder_c
style top fill:lightgreen
Loading

Use this command in the browser console to find the dups:

Object
    .entries(
        Array
            .from(document.querySelectorAll('*[id]'))
            .reduce((acc, e) => {
                acc[e.id] = acc[e.id] ?? [];
                acc[e.id].push(e);
                return acc;
            }, {})
    )
    .reduce((acc, [id, arr]) => {
        arr.length > 1 && acc.push({ [id]: arr }) && console.log([id, arr]);
        return acc;
    }, []);

Mermaid defines the "flowchart-" ids here and "L-" ids here.

Some screenshots:
lb_flowchart

lb_L

Expected behavior

No errors in the browser console.

@josevalim
Copy link
Contributor

Good find. I could not reproduce this on GH nor Mermaid's Live Editor. Both of them use a unique integer for each Mermaid element, we should most likely do the same. :)

@josevalim josevalim added the bug Something isn't working label Sep 25, 2022
@jonatanklosko
Copy link
Member

@josevalim GH loads graphs in an iframe, while in Mermaid's Live Editor I don't think we can enter multiple graphs at once to reproduce this?

One thing is that we cache the SVG content by mermaid content, however that's not really the problem here, because in the example all graphs are different. If you look at the error log, there are ids like flowchart-pointEnd and those seem to be fixed for every graph.

If you go to Mermaid flowchart docs and look for [id="flowchart-pointEnd"], there's a lot of elements matching it. There are other duplicated ids too.

@josevalim
Copy link
Contributor

@williamthome can you please report this bug in Mermaid and see what do they say?

@jonatanklosko worst case scenario we will need to wrap them in iframes too?

@jonatanklosko
Copy link
Member

@josevalim I think we could traverse the elements and replace the ids to ensure they are unique.

@williamthome
Copy link
Author

@josevalim Yes, I will do this tonight.

@dwhelan
Copy link

dwhelan commented Sep 27, 2022

I can confirm that @williamthome is correct and mermaid currently creates SVG markers with duplicate ids when there are multiple diagrams on the same html page.

The fix was to prefix the id of marker elements with the id of their parent SVG element.

I will follow up with the mermaid team to see when this will get merged (I have only recently started with mermaid and am not a contributor).

@dwhelan
Copy link

dwhelan commented Sep 27, 2022

If you decide to roll your own unique id you will need to take care to update path elements with marker urls in them. The id's of the marker elements are referenced from marker-start, marker-mid or marker-end attributes. Any changes in marker ids would need to be reflected here.

@jonatanklosko
Copy link
Member

@dwhelan thanks for the insights! Prefixing all ids sounds like the way to go, hopefully they accept it upstream.

And to handle caching we could set the id to something like mermaid__n__, then whenever we reuse the SVG we just replace it with a unique id in the string directly.

@williamthome
Copy link
Author

@dwhelan Is this your fix? Did you create the PR? mermaid-js/mermaid@develop...dwhelan:mermaid:fix/1871_lineColor_spill

@jonatanklosko jonatanklosko added the upstream The issue must be tackled upstream label Sep 15, 2023
@williamthome
Copy link
Author

Closing in favor of mermaid-js/mermaid#4825

@jonatanklosko
Copy link
Member

@williamthome perfect, thanks for checking!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working upstream The issue must be tackled upstream
Projects
None yet
Development

No branches or pull requests

4 participants