Resolves #7312 by adding endianness to packets#7585
Resolves #7312 by adding endianness to packets#7585xia-stan wants to merge 1 commit intomermaid-js:developfrom
Conversation
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@mermaid-js/examples
mermaid
@mermaid-js/layout-elk
@mermaid-js/layout-tidy-tree
@mermaid-js/mermaid-zenuml
@mermaid-js/parser
@mermaid-js/tiny
commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7585 +/- ##
==========================================
- Coverage 3.34% 3.33% -0.01%
==========================================
Files 524 535 +11
Lines 55392 56244 +852
Branches 795 820 +25
==========================================
+ Hits 1853 1877 +24
- Misses 53539 54367 +828
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
knsv-bot
left a comment
knsv-bot
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for tackling #7312, @xia-stan! 🎉 Adding endianness support is a really nice quality-of-life improvement for folks documenting little-endian protocols, and the design choice of defaulting to big to keep all existing diagrams visually identical is exactly the right call. The renderer-only blast radius is also great — it keeps the change easy to reason about.
I have a few items I'd love to see addressed before this lands. Most are small mechanical things, plus the one missing piece you already flagged (visual regression test). Let's get this across the finish line. 🙌
File triage
| Tier | Count | Files |
|---|---|---|
| Tier 1 (full) | 1 | packages/mermaid/src/schemas/config.schema.yaml |
| Tier 2 (diff + context) | 3 | diagrams/packet/renderer.ts, styles.ts, types.ts |
| Tier 3 (diff only) | 3 | demos/packet.html, docs/syntax/packet.md, src/docs/syntax/packet.md |
| Skip / verify regen | 1 | config.type.ts (regenerated from schema ✅) |
What's working well
- 🎉 [praise] The
demos/packet.htmlupdate is thorough — covering all combinations of big/little ×showBits× default/forest/dark themes is really helpful for visual sanity checks. This is exactly the kind of thing that makes future regression triage much easier. - 🎉 [praise] The decision to keep
'big'as the default and isolate the change to the renderer is exactly the right call given how widely the packet diagram is consumed downstream. Minimal blast radius, zero impact on existing diagrams.
Things to address
🔴 [blocking] Use a string-literal union for endianness, not string
packages/mermaid/src/schemas/config.schema.yaml:2354-2357
Right now endianness is declared as type: string with no enum constraint. The renderer only branches on === 'little', so any typo (e.g., endianness: 'litle' or endianness: 'lil') would silently render as big-endian with no warning. The schema should constrain this to a closed set so AJV validation catches mistakes at parse time, and TypeScript narrows the type.
The pattern to follow is SankeyLabelStyle (config.schema.yaml:2228-2238) — define a top-level type with enum and meta:enum, then $ref it from PacketDiagramConfig:
PacketEndianness:
description: |
The endianness of the packet bit ordering. 'big' renders bit 0 on the
left (default); 'little' renders bit 0 on the right.
type: string
enum:
- big
- little
meta:enum:
big: Bit 0 on the left, increasing left-to-right (network/MSB-first).
little: Bit 0 on the right, increasing right-to-left (LSB-first).
default: bigThen in PacketDiagramConfig:
endianness:
description: Endianness of the packet bit ordering when displaying bit numbers and labels.
$ref: '#/$defs/PacketEndianness'
default: bigAfter updating the schema, regenerate config.type.ts with pnpm run --filter mermaid types:build-config — you'll get endianness?: 'big' | 'little' instead of endianness?: string, which will also let TypeScript catch the dead-code issue below.
🔴 [blocking] endianness is in the wrong place in styles.ts and types.ts
packages/mermaid/src/diagrams/packet/types.ts:25 and packages/mermaid/src/diagrams/packet/styles.ts:16
The renderer reads endianness from Required<PacketDiagramConfig> (i.e., the diagram config), not from PacketStyleOptions. So the additions to PacketStyleOptions and defaultPacketStyleOptions.endianness = 'big' are dead code — they're never read by anything. PacketStyleOptions is for theme/CSS-mappable values like colors and font sizes, while endianness is a behavioral config that lives on PacketDiagramConfig (which you've already wired up correctly via config.schema.yaml).
Please remove:
- The
endianness?: string;field fromPacketStyleOptionsintypes.ts. - The
endianness: 'big',line fromdefaultPacketStyleOptionsinstyles.ts.
🔴 [blocking] Missing E2E visual regression test
The CLAUDE.md test policy requires E2E visual regression tests for any change that affects rendered SVG output, and renderer changes are the canonical case. I totally understand it's outside your usual workflow — the good news is the existing packet tests in cypress/integration/rendering/packet.spec.js are very small and you can copy the pattern almost verbatim. You'd add 2-4 cases:
- A little-endian packet with
showBits: true(verifies bit numbers swap left↔right). - A little-endian packet with
showBits: false(verifies block ordering still mirrors). - Optionally, a multi-row little-endian packet to confirm row-spanning blocks render correctly.
Each case is just an imgSnapshotTest(diagramText, options) call. The existing cypress/integration/rendering/packet.spec.js file already shows the structure. Happy to walk you through it if you'd like — just leave a comment.
You don't need to commit baseline PNGs from your machine; on the main repo, Argos handles snapshot acceptance during PR review.
🟡 [important] Missing changeset
The PR is a user-facing feature (new config option), so it needs a changeset:
pnpm changesetSelect mermaid (minor bump), and use a message prefixed with feat:, e.g.:
feat: add endianness option to packet diagram config (big/little)
🟢 [nit] Typo in demo title
demos/packet.html — "Big Endian- Dark theme" is missing the space before the dash. Should be "Big Endian - Dark theme" to match the pattern used in the other titles.
🟢 [nit] Consider a tiny pure-function unit test for the geometry
The little-endian branch in drawWord (renderer.ts:55-58) does a non-trivial coordinate flip:
const blockX = isLittleEndian
? (bitsPerRow - (block.end % bitsPerRow) - 1) * bitWidth + 1
: (block.start % bitsPerRow) * bitWidth + 1;It would be lovely to extract this to a small computeBlockX(block, bitsPerRow, bitWidth, isLittleEndian) helper at module scope and add a couple of unit tests in packet.spec.ts. That's not strictly required, but it gives you a fast-feedback safety net for edge cases (single-bit blocks, blocks at row boundaries) without depending on visual snapshots.
💡 [suggestion] Mirror the paddingX gap in little-endian
Just so you're aware: in big-endian mode the per-block paddingX gap currently sits on the right side of each block (because width = bits*bitWidth - paddingX). In little-endian mode that gap stays on the right rather than mirroring to the left, so the visual mirror isn't perfectly symmetric. This isn't necessarily wrong — it's a defensible choice — but worth confirming you intended it that way. If you'd rather mirror the gap as well, you'd shift blockX by + paddingX (or compute width and x together as a single mirror transform). Non-blocking, just flagging it for consideration.
Security
No XSS or injection issues identified. The endianness value is read from diagram config (already AJV-validated once you add the enum), and the only DOM writes are numeric coordinates and bit-number text via D3's .text() method, which escapes its argument.
Thanks again for contributing — once the schema enum, the dead code in styles.ts/types.ts, the E2E test, and the changeset are sorted, this should be ready to land. Let me know if anything's unclear and I'll be happy to help! 🚀
2 participants
📑 Summary
Adds endianness to packets.
Resolves #7312
📏 Design Decisions
The packet type is a great feature for documenting binary data payloads. However, the current implementation limits users to big endian formats. There are a number of applications that use little endian format, where the bytes display in the reverse order. These applications need a way to toggle the endianness of the packet class to display properly.
I used two primary design constraints:
I implemented 1 by having the default configuration item use big endian. This ensures that existing implementations are unaffected. I minimized code changes by isolating the functional change to just the renderer.
** Note**: I have not added the E2E visual regression test as this is a little outside of my wheelhouse / skills at the moment. I am pushing this PR by recommendation in the ticket.
📋 Tasks
Make sure you
MERMAID_RELEASE_VERSIONis used for all new features.pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.