Skip to content

fix: keep the widest nominalTraceWidth when merging connections (#1721) - #1773

Closed
DPS0340 wants to merge 2 commits into
tscircuit:mainfrom
DPS0340:fix/merge-connections-widest-trace-width
Closed

fix: keep the widest nominalTraceWidth when merging connections (#1721)#1773
DPS0340 wants to merge 2 commits into
tscircuit:mainfrom
DPS0340:fix/merge-connections-widest-trace-width

Conversation

@DPS0340

@DPS0340 DPS0340 commented Jul 25, 2026

Copy link
Copy Markdown

Addresses the root cause behind #1721, where 1.0 mm motor traces were emitted at 0.15 mm.

Root cause

mergeConnections merges every SimpleRouteConnection that shares a point into one connection, but it kept only the first nominalTraceWidth it happened to encounter:

// Take the nominalTraceWidth from the first connection for now
// A more robust solution might average or pick the max/min based on context
if (nominalTraceWidth === undefined && conn.nominalTraceWidth !== undefined) {
  nominalTraceWidth = conn.nominalTraceWidth
}

These widths are minimum requirements. When a 1 mm motor output merged with a thinner branch on the same net, whichever connection came first won — so the 1 mm requirement was silently dropped and the route fell back to the default width.

This also explains the otherwise puzzling detail in the report that the loss is branch/merge dependent rather than a global parsing failure: nets whose wide connection happened to be visited first kept 0.8 mm, and nets where a thin branch came first lost it. The outcome depended on connection order.

Fix

Take the maximum requested width across the merged group, so a merge can never narrow a trace below what any member asked for. Connections that request nothing still merge to undefined and keep the existing default behavior.

Tests

Added tests/solvers/merge-connections-nominal-trace-width.test.ts with 5 cases: widest-wins, order independence (the failing direction and the passing one), no-width-stays-undefined, a single width surviving merges with unspecified branches, and separate nets keeping their own widths.

The widest-wins case fails on main with Expected: 1, Received: 0.15 and passes here.

Verification

Full bun test: 411 pass / 13 fail, versus 406 pass / 13 fail on an unmodified checkout — same 13 pre-existing failures (bugreport fixtures hitting the 5 s timeout), plus the 5 new tests. No regressions.

Note this fixes width preservation through connection merging. If #1721 also involves width loss further down the pipeline (e.g. in TraceWidthSolver or HD route conversion), that would be a separate follow-up — happy to dig in if you can confirm from the gist data.

@vercel

vercel Bot commented Jul 25, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
capacity-node-autorouter Ready Ready Preview, Comment Jul 26, 2026 6:14am

Request Review

@tscircuitbot

Copy link
Copy Markdown
Contributor

Benchmark This PR

Run benchmarks by commenting on this PR:

/benchmark [benchmark.sh args...]
/benchmark-long [benchmark.sh args...]
/benchmark-all

Comment /benchmark to run the default dataset, or append any arguments accepted by ./benchmark.sh.
Comment /benchmark-long for an 8-vCPU run that defaults to 8 workers and has an eight-hour timeout.
Comment exactly /benchmark-all to start separate workflow runs and result comments for the default dataset plus srj18, srj19, srj20, srj21, and srj23.

Everything after /benchmark or /benchmark-long is safely forwarded to ./benchmark.sh, except --profile-solvers, which enables profile comparison tables.
Examples: /benchmark --dataset 18 --sample-timeout 2000s, /benchmark --pipeline 7 --scenario-limit 20, /benchmark all 20 --concurrency 8, and /benchmark-long --dataset 18.

Use /update-snapshots (or /us) to run BUN_UPDATE_SNAPSHOTS=1 bun test --timeout 120_000 on the PR branch and auto-commit snapshot updates.
Use /usf to read recent failed test files, update and verify their exact CI test shards, and auto-commit only their snapshots. It uses the configured fast benchmark runner by default; use /usf --ubuntu-latest for GitHub-hosted x64 CI parity.

Any PR whose title contains [BENCHMARK TEST] will automatically run one default-dataset benchmark on PR updates; it does not post a PR result comment.

The CI format-check was failing on the multi-line Math.max call.
@DPS0340

DPS0340 commented Jul 25, 2026

Copy link
Copy Markdown
Author

@imrishabh18 mind taking a look at this one when you get a chance?

2 files, +137/-8. In mergeConnections, when two connections merge, the resulting nominalTraceWidth took whichever value it hit first rather than the widest, so merging a wide trace into a narrow one silently narrowed the wide one. This keeps the maximum instead; connections with no requested width still stay undefined.

Tests cover the merge cases (5 pass). I just pushed a formatting fix — the earlier format-check failure was a multi-line Math.max that biome wanted wrapped, now clean.

@DPS0340

DPS0340 commented Jul 25, 2026

Copy link
Copy Markdown
Author

@0hmX adding you — you've merged 7 of the last ~25 non-bot PRs here, and I'd only tagged @imrishabh18. Whoever gets to it.

Small: the source change is 14 lines in mergeConnections.ts (+14/-8); the other 124 lines are the regression test.

Addresses the root cause behind #1721 (1.0 mm motor traces emitted at 0.15 mm). mergeConnections unions every SimpleRouteConnection sharing a point but kept only the first nominalTraceWidth it encountered:

// Take the nominalTraceWidth from the first connection for now
if (nominalTraceWidth === undefined && conn.nominalTraceWidth !== undefined) {
  nominalTraceWidth = conn.nominalTraceWidth
}

These are minimum requirements, so a 1 mm motor output merging with a thinner branch on the same net loses its requirement whenever the thin branch is visited first. That explains the reporter's observation that the loss is branch/merge dependent rather than a global parsing failure — it was connection order, not the value.

The fix takes the max across the merged group. Demonstrated on main:

merging keeps the widest requested nominalTraceWidth
  Expected: 1
  Received: 0.15

Full suite: 411 pass / 13 fail vs 406 pass / 13 fail on unmodified main — same pre-existing bugreport timeout failures, plus my 5 new tests. No regressions. CI green (12 checks), MERGEABLE / CLEAN.

@DPS0340

DPS0340 commented Jul 25, 2026

Copy link
Copy Markdown
Author

Process note, not code: this repo's stalebot closes PRs after 3 days of inactivity (days-before-pr-stale: 2, days-before-pr-close: 1, daily at 01:30 UTC).

That's not theoretical — in tscircuit/core, which runs the same workflow, 18 of the 29 unmerged PR closures since 2026-07-01 were github-actions[bot], more than every human closer combined. It doesn't discriminate by author either; mohan-bee, seveibar, techmannih and AnasSarkiz all had PRs closed that way.

So if this one sits, it will be closed on its own regardless of whether the fix is wanted. To avoid me bumping it purely to outrun a bot, either:

  1. add an exempt label (pinned / help-wanted) if you want it kept while it waits, or
  2. tell me it's not wanted and I'll close it myself.

For the merge decision itself, it's 14 lines of source (mergeConnections.ts) plus a 124-line regression test. It fixes #1721's root cause: merging connections kept the first nominalTraceWidth rather than the widest, so a 1 mm motor trace merging with a thin branch silently lost its width requirement depending on connection order.

CI is green (12 checks), MERGEABLE / CLEAN. @0hmX @imrishabh18

@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.

@github-actions

Copy link
Copy Markdown

This PR was closed because it has been inactive for 1 day since being marked as stale.

@github-actions github-actions Bot closed this Jul 31, 2026
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.

2 participants