Fix Human readable IDs in autorouter diagnostics#2091
Fix Human readable IDs in autorouter diagnostics#2091mohan-bee wants to merge 19 commits intotscircuit:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
seveibar
left a comment
seveibar
left a comment
There was a problem hiding this comment.
@mohan-bee why did you refactor so much? Dont refactor and fix, turn off the linter youre using- otherwise the reviewer cant see or attribute what touve changed
There was a problem hiding this comment.
Don't console.log in tests, you can inline snapshot it
|
@seveibar do i have to fix that timeout issues in the test files ? |
|
Not unless you caused them! Keep in mind the last person to merge to main have them all passing |
|
@seveibar ready for review ! |
mohan-bee
force-pushed
the
fix-autorouter-diagnostics
branch
from
30ea7a0 to
ac7a74f
Compare
| sourcePort.source_component_id as string, | ||
| ) | ||
| return `${sourceComponent?.name ?? "unknown"}.${sourcePort.name}` | ||
| } |
There was a problem hiding this comment.
import this method from @tscircuit/circuit-json-util instead
| const readablePortA = getReadableName(db, portA.pcb_port_id) | ||
| console.error( | ||
| `(source_port_id: ${portA.source_port_id}) for trace ${trace.source_trace_id} does not have x/y coordinates. Skipping this trace.`, | ||
| `(pcb_port_id: ${readablePortA ?? portA.pcb_port_id}) for trace ${ |
There was a problem hiding this comment.
the text is literally wrong, it's not a pcb port id. Also you can commit to the readable name, remove the ??
tests/utils/autorouting/simple-route-json-readable-names.test.tsx
Outdated
Show resolved
Hide resolved
MustafaMulla29
left a comment
MustafaMulla29
left a comment
There was a problem hiding this comment.
It should be source_port_id not pcb_ port_id
|
No, I think it should be source_port coz every other place it's source_port. Confirm with @seveibar |
|
@MustafaMulla29 Since autorouting is a layout problem, pcb_port_id is more precise than source_port_id, which can map to multiple physical locations. |
| : "unknown" | ||
| console.error( | ||
| `(source_port_id: ${portB.source_port_id}) for trace ${trace.source_trace_id} does not have x/y coordinates. Skipping this trace.`, | ||
| `(${readablePortB}) for trace ${trace.source_trace_id} does not have x/y coordinates. Skipping this trace.`, |
There was a problem hiding this comment.
why use the trace id? Trace ids aren't readable- can you use a readable name for the trace?
| portB.y === undefined | ||
| ) { | ||
| const readablePortB = portB.pcb_port_id | ||
| ? getReadableNameForPcbPort(db.toArray(), portB.pcb_port_id) |
There was a problem hiding this comment.
why aren't you using portB.getReadableName() or whatever
There was a problem hiding this comment.
db.toArray() has a huge performance penalty
There was a problem hiding this comment.
since they are plain objects from db list they don't have methods.
It's not about being precise, it's about convention. |
seveibar
left a comment
seveibar
left a comment
There was a problem hiding this comment.
This shouldn't even be a console.error, we use circuit json errors for errors, we don't use spys or mocks
|
@mohan-bee you're doing good stuff btw, it's just hard to contribute to core |
|
@seveibar yes, i will learn things and reflect on the comments. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
3 participants
The autorouter was spitting out IDs like source_trace_0 and source_port_1 in error messages useful to no one.
This PR replaces those with actual net and component names so that when routing fails, you can tell what failed and where.
AI agents now get clean, readable logs pointing directly to the collapsed net or component.
Also hardened the table lookups with defensive guards and empty-list fallbacks so the autorouter no longer crashes on sparse or incomplete circuit JSON.