Skip to content

fix: ensure deterministic unnamed subcircuit connectivity keys#2332

Merged
rushabhcodes merged 13 commits into
tscircuit:mainfrom
rushabhcodes:fix/deterministic-unnamed-subcircuit-connectivity-keys
May 27, 2026
Merged

fix: ensure deterministic unnamed subcircuit connectivity keys#2332
rushabhcodes merged 13 commits into
tscircuit:mainfrom
rushabhcodes:fix/deterministic-unnamed-subcircuit-connectivity-keys

Conversation

@rushabhcodes
Copy link
Copy Markdown
Contributor

@rushabhcodes rushabhcodes commented May 24, 2026

This pull request improves the determinism and correctness of how unnamed subcircuit connectivity map keys are generated for groups, traces, ports, and nets. The logic now consistently uses a combination of subcircuit_id, source_group_id, or a fallback render ID to ensure that keys are stable and unique across renders. Existing test fixtures and outputs are updated to match the new key format, and a new test verifies that the generated keys remain deterministic.

Key changes include:

Connectivity Map Key Generation Improvements:

  • Updated the logic in Group_doInitialSourceAddConnectivityMapKey to generate subcircuit_connectivity_map_key using subcircuitName ?? unnamedsubcircuit${group.subcircuit_id ?? group.source_group_id ?? group._renderId} instead of relying solely on _renderId. This ensures keys are more stable and meaningful, especially for unnamed subcircuits. [1] [2] [3]

Testing and Validation:

  • Added a new test (group-unnamed-subcircuit-connectivity-map-key.test.tsx) that renders circuits with unnamed subcircuits multiple times and asserts that the generated connectivity map keys are deterministic and consistent across renders.

Fixture and Test Data Updates:

  • Updated all relevant test fixtures and JSON assets (such as simple-circuit.json) to use the new, deterministic subcircuit_connectivity_map_key format, replacing previous keys that used only the render ID. [1] [2] [3] [4]

These changes make subcircuit connectivity mapping more robust and predictable, reducing the likelihood of key collisions or inconsistencies between renders.

Copilot AI review requested due to automatic review settings May 24, 2026 19:45
@vercel
Copy link
Copy Markdown

vercel Bot commented May 24, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
tscircuit-core-benchmarks Ready Ready Preview, Comment May 27, 2026 5:08pm

Request Review

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aims to make subcircuit_connectivity_map_key deterministic for unnamed subcircuits by removing the use of the runtime-derived group._renderId, improving SVG/snapshot stability across renders.

Changes:

  • Replaces _renderId-based fallback connectivity key generation with a deterministic identifier based on group.subcircuit_id / group.source_group_id.
  • Applies the new deterministic fallback consistently for traces, ports, and nets in subcircuits.
Comments suppressed due to low confidence (2)

lib/components/primitive-components/Group/Group_doInitialSourceAddConnectivityMapKey.ts:71

  • Same issue here: ?? "" can produce an empty unnamed-subcircuit suffix, leading to ambiguous/colliding connectivity keys. Prefer requiring a stable identifier (e.g., subcircuit_id or source_group_id) instead of falling back to an empty string.
    const connectivityMapKey = `${subcircuitName ?? `unnamedsubcircuit${group.subcircuit_id ?? group.source_group_id ?? ""}`}_${connNetId}`

lib/components/primitive-components/Group/Group_doInitialSourceAddConnectivityMapKey.ts:99

  • Same issue here: falling back to an empty string for the unnamed subcircuit identifier can create duplicate keys across subcircuits. It’s safer to require subcircuit_id/source_group_id to be present than to emit a potentially-colliding key.
    const connectivityMapKey = `${subcircuitName ?? `unnamedsubcircuit${group.subcircuit_id ?? group.source_group_id ?? ""}`}_${connNetId}`

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@rushabhcodes rushabhcodes requested review from a team, Abse2001, ShiboSoftwareDev, imrishabh18 and seveibar and removed request for seveibar May 24, 2026 19:50
Copy link
Copy Markdown
Contributor

@seveibar seveibar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be much better if you didn't use ids. Also you should have a test that uses toMatchInlineSnapshot so that we can see the id that's generated

@seveibar
Copy link
Copy Markdown
Contributor

seveibar commented May 24, 2026

I think it's ok to use ids in this case, but ideally we find a better key using something like _getRenderPathKey() or something like that that returns the full path hierarchy e.g. root.group0

@rushabhcodes
Copy link
Copy Markdown
Contributor Author

currently this return something like unnamedsubcircuitsubcircuit_source_group_0_connectivity_net0

@github-actions
Copy link
Copy Markdown

This PR has been automatically marked as stale because it has had no recent activity. It will be closed if no further activity occurs.

@rushabhcodes rushabhcodes force-pushed the fix/deterministic-unnamed-subcircuit-connectivity-keys branch from 93e2f90 to 3d789ec Compare May 27, 2026 13:07
Copy link
Copy Markdown
Contributor

@seveibar seveibar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see important comment

@rushabhcodes rushabhcodes merged commit 7b03bb6 into tscircuit:main May 27, 2026
11 checks passed
@rushabhcodes rushabhcodes deleted the fix/deterministic-unnamed-subcircuit-connectivity-keys branch May 27, 2026 17:26
@tscircuitbot
Copy link
Copy Markdown
Contributor


Thank you for your contribution! 🎉

PR Rating: ⭐⭐⭐
Impact: Major

Track your contributions and see the leaderboard at: tscircuit Contribution Tracker


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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants