fix(class): support styling and callbacks for generic classes#7762
fix(class): support styling and callbacks for generic classes#7762Dharya-dev wants to merge 3 commits into
Conversation
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🦋 Changeset detectedLatest commit: 9d8ad46 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@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 #7762 +/- ##
==========================================
- Coverage 3.26% 3.26% -0.01%
==========================================
Files 599 599
Lines 60766 60776 +10
Branches 917 917
==========================================
Hits 1986 1986
- Misses 58780 58790 +10
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
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for this, @Dharya-dev — really clean, well-scoped bug fix. 🎉 You correctly traced #7753 to its root cause: classes are stored in this.classes keyed by the bare className (addClass → classDb.ts:114-122), but the interactivity/style setters were looking them up with the full generic id (C1~T~), so the lookup silently missed for generic classes.
I checked this out locally and verified the behaviour:
- Full class spec passes with the fix (382 tests green).
- TDD check: reverting the source while keeping your new tests makes all 3 new tests fail, and they pass with the fix — so the tests genuinely capture the reported bug. 🎉
- Ran a dedicated XSS/injection pass over the changes: no issues. The normalized id flows through
splitClassNameAndType→common.sanitizeText(it's actually double-sanitized) and is only used as aMapkey, thesecurityLevel !== 'loose'guard insetClickFuncis untouched, andsanitizeText/utils.formatUrlon tooltip/link are intact.
What's working well
- 🎉 [praise] Reusing the existing
splitClassNameAndTypehelper rather than ad-hoc~-stripping keeps this consistent with how the rest of the file already normalizes ids (setClassLabel:100,addRelation:239-240,addClassesToNamespace:662). - 🎉 [praise] Replacing the fragile
this.classes.get(id)!non-null assertions insetTooltipandsetClickEventwithif (classNode)guards is a nice defensive improvement beyond the literal bug — a missing class id now no-ops instead of throwing. - 🎉 [praise] Changeset present, correct
patchbump withfix:prefix, and the PR links the issue.
Things to address
-
🟡 [important] — Test coverage gap for
setTooltipandsetLink. The fix touches five code paths (setCssClass,setTooltip,setLink,setClickEvent,setClickFunc), but the new tests only coversetCssClass(explicit +:::shorthand) and the click/callback path.setTooltipandsetLinkon a generic class have no regression test, even though they had the exact same defect. It would be great to add two small tests so the full scope is locked down, e.g.:- tooltip:
class C1~T~+click C1~T~ call cb() "a tip"→ assertclassDb.getClass('C1').tooltipis set - link:
class C1~T~+link C1~T~ "https://example.com"→ assertgetClass('C1').linkis set andcssClassesincludesclickable
- tooltip:
-
🟢 [nit] Minor stylistic inconsistency within the diff:
setCssClass/setLinkreassignid = this.splitClassNameAndType(id).className;in place, whilesetTooltip/setClickEventintroduce a separateclassNameconst for the same operation. Both read fine — only worth tidying if you're already touching these lines. -
🟢 [nit] The snapshot change
domId: "classId-Student-149"→"classId-Student-152"is just churn from the module-levelclassCounterbeing bumped by the new test classes added earlier in the file. The update is correct and expected — flagging only so it's clear this isn't an unintended behaviour change. -
💡 [suggestion] — out of scope, no action needed here. In
setCssClass/setLinktheif (/\d/.exec(_id[0]))digit-prefix branch runs beforesplitClassNameAndType, whereasaddClasskeys classes purely bysplitClassNameAndType(...).classNamewith no digit prefix. So a class that is both digit-leading and generic (class 1C~T~:::x) would still not resolve. This is a pre-existing inconsistency unrelated to #7753 — possible follow-up, not something to fix in this PR.
The fix itself is correct and well-targeted; the only real ask is rounding out regression coverage for the two untested setters. Let's get this across the finish line. 🙏
c9bf018 to
0e4d733
Compare
Co-authored-by: Dharya-dev <Dharya-dev@users.noreply.github.com>
Adds missing test cases for setTooltip and setLink on generic classes (e.g. C1~T~) to cover the code paths introduced in the earlier fix. Updates the inline snapshot domId to account for the two new test-generated classes incrementing the module-level counter.
0e4d733 to
cfd2391
Compare
|
Hi maintainers! 👋 Just wanted to give this a friendly bump. The recent commits add the required unit tests and address the coverage gaps. The PR should be ready for review. Please let me know if there are any other changes needed! |
kedar49
left a comment
There was a problem hiding this comment.
splitClassNameAndType() normalization logic is now duplicated across multiple methods (setCssClass, setTooltip, setClickEvent, setClickFunc). It may be worth centralizing generic-class resolution into a helper to avoid future inconsistencies.
knsv-bot
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for the update, @Dharya-dev — this looks good to me now. 🎉
The latest changes round out the regression coverage for the full set of generic-class interaction paths: explicit cssClass, shorthand :::, callbacks, tooltips, and links. I also re-checked the implementation against #7753: class declarations with generic suffixes are stored under the normalized class name (C1), and the affected setters now normalize C1~T~ through splitClassNameAndType(...) before looking up the class node.
What's working well
- 🎉 [praise] The fix stays nicely scoped to the class diagram DB paths that were doing class lookups with generic IDs.
- 🎉 [praise] The new tests now cover all five changed behavior paths: CSS class assignment, shorthand CSS class assignment, callback binding, tooltip binding, and link binding.
- 🎉 [praise] Replacing the previous non-null assertions in the click/tooltip paths with guarded lookups makes this a little more defensive without broadening the PR.
- 🎉 [praise] Changeset is present and correctly marks this as a patch-level
fix(class)change.
Security
No XSS or injection issues identified.
I checked the changed paths specifically:
- tooltip text still goes through
sanitizeText(...) - links still go through
utils.formatUrl(...) - callback execution remains gated behind
securityLevel === 'loose' - the new generic normalization only affects class lookup keys and does not introduce a new DOM/SVG sink
Things to address
Nothing blocking from my side.
One tiny non-blocking note: setCssClass / setLink mutate the local id variable, while setTooltip / setClickEvent introduce a className const for the same normalization step. Both are clear enough, so I wouldn’t ask for a change here unless you’re already touching those lines.
Looks ready to merge. 🙌
📑 Summary
Hey everyone! I noticed an issue where generic classes (like
Class~T~) in class diagrams weren't receiving CSS styles or callback events properly. The parser was picking them up fine, but they weren't being normalized (stripping out the~T~) before assigning the styles or interactivity handlers inclassDb.ts. This PR fixes that!Resolves #7753
📏 Design Decisions
classDb.ts(specificallysetCssClass,setTooltip,setLink, andsetClickEvent) to normalize the class IDs. I used the existingsplitClassNameAndTypehelper to strip the generic signatures before updating the class nodes in the DB.classDiagram.spec.tsto make sure styles apply correctly through both explicit class statements and shorthand syntax, and to verify the callback bindings on generic classes.📋 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:.