Mermaid ADR approval and site implementation - #834
Conversation
withinfocus
left a comment
There was a problem hiding this comment.
Let's split the ADR and plan from the application.
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the Mermaid diagram-standard adoption: ADR 0033 acceptance, the new Contributing › Diagrams standard page, Docusaurus wiring (theme-mermaid, ELK layout, an inline Code Review DetailsNo new findings. Notes on prior review threads (already resolved by the author):
Verified independently:
Dependency Changes
|
not sure what you mean by this. Are you saying to split the diagram updates from the ADR? |
Yes. We want to merge the ADR in a Proposed status before Council. |
8b5e95b to
465befb
Compare
theMickster
left a comment
There was a problem hiding this comment.
A couple improvements requested and one question. great start!
This is a toy example where, presumably, `username` had a NOT NULL constraint as the real `email` column does, but this makes it explicit that these are concerns for these kinds of migrations.
withinfocus
left a comment
There was a problem hiding this comment.
Still a draft technically, but saw a few tweaks.
| # Diagram standard | ||
|
|
||
| **Audience:** Bitwarden engineers, contributors, and AI agents authoring or reading diagrams | ||
| anywhere Bitwarden documents its systems. | ||
|
|
||
| ## Notation | ||
|
|
||
| - The keywords MUST, MUST NOT, SHOULD, SHOULD NOT, and MAY are to be interpreted as described in | ||
| [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119). | ||
|
|
There was a problem hiding this comment.
⛏️ This is rather "official" to me and comes off as a bit too formal. It's a bit AI-like too, as if it's instructions with the all-caps language. We aren't writing a spec here as I see it.
There was a problem hiding this comment.
The notation section also has a single bullet point that renders awkwardly.
There was a problem hiding this comment.
Both of these things hint at an upcoming documentation standard I have drafted, but split this out of for speed.
Adopting a well known standard for communication is in line with our standards best practices, RFCs do this for a reason to be explicit about what is allowed or required. This phrasing is basically the same as the RFC it references.
Removing the single bullet feels fine, it's a holdover from when there were more and because the draft documentation standard does have multiple
| sidebar_position: 2 | ||
| --- | ||
|
|
||
| # Diagram standard |
There was a problem hiding this comment.
🎨 I think this can just be "Diagrams".
There was a problem hiding this comment.
It's called that because it's referenced as a standard in the ADR and I'd like it to be seen as one. things that go against this doc go against "the standard"
| "@docusaurus/theme-mermaid": "3.10.1", | ||
| "@mdx-js/react": "3.1.0", | ||
| "@mermaid-js/layout-elk": "0.1.9", |
There was a problem hiding this comment.
❓ QUESTION: Was AppSec approval obtained for the new @docusaurus/theme-mermaid and @mermaid-js/layout-elk dependencies?
Context
These are net-new dependencies (replacing remark-kroki). Bitwarden's Dependency Review and Approval process asks that new dependencies go through AppSec review before integration, and the PR description does not reference a VULN task or approval. Both are first-party official packages (MIT-licensed per the lockfile), so this is likely low-risk — just confirming the process was followed or intentionally waived for this internal docs site.
There was a problem hiding this comment.
Ran this through a security review directly. Low risk on the two direct packages.
Direct packages (both green):
@docusaurus/theme-mermaid@3.10.1— MIT, first-party Docusaurus org, version-locked to@docusaurus/core@3.10.1already in the tree. No install scripts.@mermaid-js/layout-elk@0.1.9— MIT, official Mermaid-JS org. Note it's pre-1.0 (0.x), so API stability isn't guaranteed.
Licensing: layout-elk pulls in elkjs@0.9.3, which is EPL-2.0 (Eclipse Public License, weak copyleft), not MIT. For a docs site consuming the unmodified npm package at build time this is fine, but it's a different license class worth noting for the record.
The actual change is transitive: the two-line package.json diff drives +1151/-200 in the lockfile, nearly all of it mermaid@11.16.0 and its subtree (dompurify, katex, cytoscape, d3, marked, roughjs, uuid@14.0.1, and more). That subtree is the attack-surface change, not the two direct adds.
Vuln scan: no open Dependabot alert touches the new subtree. The one open uuid advisory (GHSA-w5hq-g745-h8pq, affects < 11.1.1) doesn't apply here, since mermaid resolves uuid@14.0.1.
Reviewed and approved. The direct packages are first-party and low-risk, the EPL-2.0 transitive is acceptable for build-time use on an internal docs site, and the new subtree carries no open advisories. No VULN task required for this one.
withinfocus
left a comment
There was a problem hiding this comment.
One minor correction Claude found, but then I can approve.
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
withinfocus
left a comment
There was a problem hiding this comment.
Great change showing Mermaid usage!
📔 Objective
Adopts a Bitwarden-wide diagram standard: Mermaid source in Markdown, one diagram per audience/intent/scope, sources living beside the docs they illustrate.
Motivation (from ADR 0033 in this PR):
What this PR contains:
Suggested Review Guidelines
This is a big PR, I know. Please read the ADR and Diagram Standard carefully. They're concise and important. Everything else is updating diagrams to comply with these standards (with the exception writing perspectives).
Use the below link table to review diagram updates for accuracy. Meaningful changes have been made to EDD and push notifications, please give those added attention.
📸 Screenshots
Old vs. new for each converted or adjusted page.