Skip to content

fix: make hole circuit json parse against circuit-json schemas (#2843)#2844

Open
DPS0340 wants to merge 1 commit into
tscircuit:mainfrom
DPS0340:fix/null-pcb-component-id
Open

fix: make hole circuit json parse against circuit-json schemas (#2843)#2844
DPS0340 wants to merge 1 commit into
tscircuit:mainfrom
DPS0340:fix/null-pcb-component-id

Conversation

@DPS0340

@DPS0340 DPS0340 commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Closes #2843.

Two independent defects that both make core emit circuit JSON which fails any_circuit_element.parse().

                                          before   after
board-level <hole />                      invalid  valid
board-level <platedhole />                invalid  valid
<chip footprint="dip8" /> plated holes    invalid  valid

1. pcb_component_id: null on board-level holes

PrimitiveComponent initialises ids to null (pcb_component_id: string | null = null), and the holes resolve their owner with ??, which only falls through on undefined. A hole inside a footprint has an owner; one placed directly on the board doesn't, so null reached circuit JSON — and every id on these schemas is z.string().optional(), which rejects null.

Isolated it before fixing, so the diagnosis isn't a guess:

as-is                                     → false
{ ...hole, pcb_component_id: undefined }  → true
omitted entirely                          → true

Fixed with a small optionalId helper at the two sites where the schema actually permits an absent id.

2. circular_hole_with_rect_pad missing hole_shape / pad_shape

Found while verifying the first fix — the test still failed, and the culprit was a different element. That schema variant requires both fields:

var pcb_circular_hole_with_rect_pad = z.object({
  shape: z.literal("circular_hole_with_rect_pad"),
  hole_shape: z.literal("circle"),
  pad_shape: z.literal("rect"),
  ...
})

PlatedHole.ts omitted them at that one insertion site while the neighbouring pill variants in the same file already set them, so I matched the existing pattern. A plain <chip footprint="dip8" /> was enough to trigger this.

Deliberately out of scope

The sweep also found pcb_silkscreen_text and pcb_fabrication_note_text carrying pcb_component_id: null. I tried the same normalisation there and tsc rejected it — those schemas require a non-optional string. That makes it a design question (what should a board-level silkscreen text reference?) rather than a null→undefined fix, so I reverted it and reported it in #2843 instead of guessing.

Verification

The test bites. Reverting Hole.ts + PlatedHole.ts:

Expected: > 0
Received: 0
(fail) board-level holes produce valid circuit json

The test asserts both directions: board-level holes have no id, footprint holes keep theirs (typeof === "string"), and every hole passes any_circuit_element.safeParse. So it can't be satisfied by blanket-clearing the field.

Suite: 1260 pass / 0 fail, tsc --noEmit 0 errors, biome format clean.

One inline snapshot changed — plated-hole-pill-rotated2 had "pcb_component_id": null recorded, i.e. the bug was captured in an expectation. It now reads undefined.

Worth noting how this was found: validating core's own output against circuit-json's exported schemas across several board shapes. It's cheap and it also surfaced pcb_group.anchor_alignment: null and schematic_group.subcircuit_id: null, which I've left for separate triage.

@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)
tscircuit-core-benchmarks Ready Ready Preview, Comment Jul 25, 2026 5:11pm

Request Review

@DPS0340

DPS0340 commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Housekeeping — rebased onto current main (d99c99f), still 1260 pass / 0 fail. No behaviour change.

Since I have several PRs open here now, a note on how they interact so none of this lands on you:

Five of the seven are fully independent — they touch different files and merge in any order:

PR file
#2838 Inductor.ts
#2844 Hole.ts, PlatedHole.ts
#2846 Group/Group.ts (group insert sites)
#2840 Group/Group.ts (pcb_trace segment insert)
#2842 NormalComponent.ts, Group/Group.ts, Group_doInitialPcbComponentAnchorAlignment.ts

Two pairs conflict, and both are trivial:

  1. fix: omit the voltage half of the fuse label when voltageRating is absent (#2833) #2834fix: honour schShowRatings={false} on fuse (#2835) #2836 — both edit Fuse._getSchematicSymbolDisplayValue(). Whichever merges first, the other needs the two guards stacked:

    if (this._parsedProps.schShowRatings === false) return undefined   // #2836
    if (voltage === undefined || Number.isNaN(voltage)) return currentDisplay  // #2834
    return `${currentDisplay} / ${formatSiUnit(voltage)}V`

    They're orthogonal (one suppresses the whole label, the other drops just the voltage half), so the combined behaviour is well-defined. Both test files also touch fuse.test.tsx, appending separate tests.

  2. fix: carry net highlightColor into pcb_trace.highlight_color (#2839) #2840fix: write display_offset_x/y as display strings (#2841) #2842 — both add a field to nearby inserts in Group.ts; the resolution is to keep both lines.

Happy to do the merge myself: say the word and I'll combine any subset into a single PR, or rebase the losers as soon as the first one lands. I'd rather you not spend time on conflict resolution for my changes.

I verified the above rather than assuming it — checked all 21 branch pairs for conflicts and confirmed each rebases cleanly onto d99c99f.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

board-level holes and DIP plated holes emit circuit JSON that fails any_circuit_element.parse()

1 participant