Skip to content

fix(utils): avoid id collisions in InitIDGenerator (preserve deterministic behavior)#7540

Open
kacemabidi12 wants to merge 2 commits intomermaid-js:developfrom
kacemabidi12:fix/initidgenerator-compat
Open

fix(utils): avoid id collisions in InitIDGenerator (preserve deterministic behavior)#7540
kacemabidi12 wants to merge 2 commits intomermaid-js:developfrom
kacemabidi12:fix/initidgenerator-compat

Conversation

@kacemabidi12
Copy link
Copy Markdown

This PR fixes a rare bug in ID generation.

Before, the non-deterministic generator used Date.now() directly, so if several IDs were created within the same millisecond, duplicates could happen. To fix this, the generator now uses the timestamp plus a small counter for calls made in the same millisecond. This keeps the IDs unique.

The deterministic generator was not changed. It still starts from seed.length so existing behavior and tests stay the same.

Files changed:

  • utils.ts
  • utils.spec.ts

Why?

  • Using only Date.now() could create duplicate IDs when many IDs were generated very quickly.
  • This could lead to problems like duplicate DOM IDs and rendering issues.
  • This update prevents that by making sure each generated ID is unique, even within the same millisecond.

Behavior notes:

  • Non-deterministic IDs are still numeric
  • They now follow: timestamp * 1000 + counter
  • Deterministic IDs still begin at seed.length
  • This is a low-risk change because it only updates the internal ID generation logic

Tests:

  • Added or updated tests for:
    -- uniqueness of non-deterministic IDs when generating many IDs quickly
    -- deterministic ID generation increasing by 1 starting from seed.length

Local testing:

  • I tested the change locally and kept deterministic initialization at seed.length to preserve the existing behavior and tests.

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Mar 27, 2026

⚠️ No Changeset found

Latest commit: f164177

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@netlify
Copy link
Copy Markdown

netlify bot commented Mar 27, 2026

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit f164177
🔍 Latest deploy log https://app.netlify.com/projects/mermaid-js/deploys/69c6ed18f2cbda00086e94d3
😎 Deploy Preview https://deploy-preview-7540--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@github-actions github-actions bot added the Type: Bug / Error Something isn't working or is incorrect label Mar 27, 2026
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Mar 27, 2026

Open in StackBlitz

@mermaid-js/examples

npm i https://pkg.pr.new/@mermaid-js/examples@7540

mermaid

npm i https://pkg.pr.new/mermaid@7540

@mermaid-js/layout-elk

npm i https://pkg.pr.new/@mermaid-js/layout-elk@7540

@mermaid-js/layout-tidy-tree

npm i https://pkg.pr.new/@mermaid-js/layout-tidy-tree@7540

@mermaid-js/mermaid-zenuml

npm i https://pkg.pr.new/@mermaid-js/mermaid-zenuml@7540

@mermaid-js/parser

npm i https://pkg.pr.new/@mermaid-js/parser@7540

@mermaid-js/tiny

npm i https://pkg.pr.new/@mermaid-js/tiny@7540

commit: f164177

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 27, 2026

Codecov Report

❌ Patch coverage is 0% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 3.34%. Comparing base (e9d4c11) to head (f164177).
⚠️ Report is 194 commits behind head on develop.

Files with missing lines Patch % Lines
packages/mermaid/src/utils.ts 0.00% 17 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #7540      +/-   ##
==========================================
- Coverage     3.34%   3.34%   -0.01%     
==========================================
  Files          524     524              
  Lines        55256   55271      +15     
  Branches       795     795              
==========================================
  Hits          1850    1850              
- Misses       53406   53421      +15     
Flag Coverage Δ
unit 3.34% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/mermaid/src/utils.ts 17.71% <0.00%> (-0.47%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kacemabidi12
Copy link
Copy Markdown
Author

I made a small change (InitIDGenerator) and added unit tests. The CI failure looks unrelated because the E2E job failed while uploading screenshots to Argos:

  • Error: You have reached the maximum screenshot capacity included in your Pro open source xl Plan. Please upgrade your Plan.

The Argos upload error caused the Cypress job to crash and produced "Could not find Cypress test run results".

  • Could a maintainer re-run the failing workflow (or skip visual tests for this PR)?

The change itself only affects unit tests in packages/mermaid and shouldn't impact visual E2E tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Bug / Error Something isn't working or is incorrect

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant