Skip to content

fix: omit group anchor_alignment and subcircuit_id instead of writing null (#2845)#2846

Open
DPS0340 wants to merge 1 commit into
tscircuit:mainfrom
DPS0340:fix/group-schema-nulls
Open

fix: omit group anchor_alignment and subcircuit_id instead of writing null (#2845)#2846
DPS0340 wants to merge 1 commit into
tscircuit:mainfrom
DPS0340:fix/group-schema-nulls

Conversation

@DPS0340

@DPS0340 DPS0340 commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Closes #2845.

<group /> wrote null into two fields that circuit-json types as string-or-absent, so both elements failed validation:

                                    before                    after
pcb_group.anchor_alignment          null (rejected)           undefined → schema default "center"
schematic_group.subcircuit_id       null (rejected)           undefined

Why undefined rather than a value

pcb_group.anchor_alignment      ninePointAnchor.default("center")
schematic_group.subcircuit_id   z.string().optional()

For anchor_alignment, omitting is strictly better than substituting "center" in core: the schema already supplies that default, so leaving it out keeps a single source of truth and stays correct if the default ever changes. The explicit pcbAnchorAlignment prop still passes through unchanged — the test asserts a "top_left" group survives, so this can't be satisfied by blanket-clearing the field.

For subcircuit_id, the ! assertion was hiding that the field is string | null = null on the component. A group outside any subcircuit genuinely has none, and undefined is what the schema expects.

Three existing tests recorded the invalid values

That's why nothing caught this — the bug was baked into expectations:

  • group-outline.test.tsx"anchor_alignment": null
  • group-subcircuit-id.test.tsx"anchor_alignment": null
  • group-schematic-box.test.tsx"subcircuit_id": null

All three updated to undefined. No assertion was weakened; they're inline snapshots of emitted values.

Verification

The test bites. Reverting Group.ts:

Received: null
(fail) groups do not emit null anchor_alignment / subcircuit_id

It asserts neither field is null, that an explicitly requested alignment survives, and that neither field appears in safeParse issues — so it pins the schema contract rather than just the absence of null.

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

Note

Locating one of the three failures took a moment because five test files share the test name "group schematic box" — a single-file rerun of the wrong one passes and looks like flakiness. I isolated it by running each candidate file separately.

This is the third and final batch from validating core's output against circuit-json's exported schemas (#2841 display_offset_x/y, #2843 hole elements). With this merged, <group /> no longer contributes violations.

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

group emits null anchor_alignment and subcircuit_id, failing circuit-json validation

1 participant