fix(drawing-2d): resolve override subtypes from the schema, not a drifted table - #3262
Conversation
…fted table (#3260) `ifcTypeCriterion` defaults `includeSubtypes` to true, so a rule naming a supertype should style everything beneath it. The expansion ran off a hand-written `IFC_TYPE_HIERARCHY` that had fallen behind the schema: `IfcBuildingElement` reached 21 of IFC4's 31 subtypes, and `IfcDistributionElement` reached 2 of 76 because neither of the two names it listed was itself a key. Elements still drew, just without the override the rule asked for, and nothing warned. The table moves to `ifc-type-hierarchy.ts` as the direct-children map of everything under `IfcElement` and `IfcSpatialElement`, derived from IFC4 ADD2 TC1. `rule-engine.ts` was at its exact module-size budget, so the table could not grow in place; its row drops 561 -> 523. `IfcFlowElement` was a key and is not an IFC entity in IFC2X3, IFC4 or IFC4X3, so it was never a legacy alias. It is kept in an explicit `AUTHORING_ALIASES` map pointing at the real supertype `IfcDistributionFlowElement` rather than deleted, since a rule may already name it. `IfcStair` -> `IfcStairFlight` and `IfcRamp` -> `IfcRampFlight` join it there: IFC4 makes the flights siblings, and narrowing those rules would be its own regression. `getIfcSubtypes` de-duplicates and tracks visited nodes so an alias pointing back into the table cannot spin. The module had no tests. Adds a behavioural suite driving rules through `applyOverrides` against named required entities, and a parity suite that re-derives the hierarchy from `@ifc-lite/data`'s `ENTITIES_IFC4` — an authority independent of the parser registry the table was generated from — failing if the table omits a subtype, invents an entity, or claims an edge the schema lacks. Verified the guard fires: deleting `IfcCurtainWall` from the table reds it with "IfcBuildingElement does not reach: IfcCurtainWall". `@ifc-lite/data` is a devDependency; the published bundle is unchanged.
|
Warning Review limit reached
This review includes 8 billable files and costs up to $2.00. Or wait 59 minutes for your next included review. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Viewer benchmark✅ No threshold regressions detected. 01_Snowdon_Towers_Sample_Structural(1).ifcBaseline recorded 2026-07-01T20:31:05.538Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
AC20-FZK-Haus.ifcBaseline recorded 2026-07-01T20:30:59.972Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
Refresh the baseline from a CI run: dispatch the Benchmark workflow with |
Conflicted only on scripts/module-size-allowlist.txt and check-module-size.mjs. Every open PR in this batch re-pinned the digest from the same base value, so whichever landed first conflicted the rest; #3196 landing on main is what did it here. Resolved to main's structure, then re-derived the budgets and the digest from `node scripts/check-module-size.mjs` rather than hand-picking them, so the allowlist states measured counts rather than a merge artefact.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
Merged Why it conflicted. Eight PRs in tonight's batch re-pin How I resolved it. Took main's structure, then re-derived every number from The digest re-pin is now correct for main as of |
Conflicted only on scripts/module-size-allowlist.txt and check-module-size.mjs. Every open PR in this batch re-pinned the digest from the same base value, so whichever landed first conflicted the rest; #3196 landing on main is what did it here. Resolved to main's structure, then re-derived the budgets and the digest from `node scripts/check-module-size.mjs` rather than hand-picking them, so the allowlist states measured counts rather than a merge artefact.
Conflicted only on scripts/module-size-allowlist.txt and check-module-size.mjs. Every open PR in this batch re-pinned the digest from the same base value, so whichever landed first conflicted the rest; #3196 landing on main is what did it here. Resolved to main's structure, then re-derived the budgets and the digest from `node scripts/check-module-size.mjs` rather than hand-picking them, so the allowlist states measured counts rather than a merge artefact.
|
CodeRabbit CLI at head: one finding, severity major, and I am rejecting it with evidence. The finding: remove the non-schema authoring aliases from the exported API — The guideline is real. The premise is not. The concern is that aliases contaminate the schema-derived table. They cannot, because the two are separate exports and BOTH are pinned: So the schema half is held to the schema in both directions, and the alias half cannot grow silently — a fourth alias fails And the remedy would cause a silent narrowing, which is the failure mode this repo has spent tonight removing.
No change made. The aliases are deliberate backward compatibility, isolated from the derived table, individually documented with why, and pinned to exactly three. On the change itself: this is the ninth instance tonight of a hand-kept table standing in for a schema relation, and it is fixed the right way — derive Verified on the merged tree against current main: |
Adversarial review of #3262 — RED verified and unusually clean; one scope gap worth namingWhat I ran (own worktree, branch head, after (The lockfile change is correct — RED verified by inverse edit. Rather than trust the changeset's account of the drift, I reproduced it: deleted the Restored, This is the best-shaped test suite I have reviewed tonight. One assertion per entity means the failure output is the bug report; the changeset's numbers ("21 of 31", "2 of 76") are checkable rather than asserted; and both controls are present and meaningful — Finding — the guarantee holds for IFC4 only, and the module doc claims more (medium)
(scratch file removed; tree clean.)
The IFC2X3 gaps are smaller but real, and one of them is live in this repo right now: I am not asking for this in the PR; it is a clear improvement as it stands and the table's IFC4 provenance is stated honestly in the changeset. What I would change is the module doc sentence above, which promises a guarantee one schema wide as though it were general, and the changeset, which never mentions that IFC4X3 and IFC2X3 rules still silently under-match. If the intent is IFC4-only for now, saying so costs a line and stops the next reader assuming the drift problem is solved. If you want it closed, Question —
|
…lves (#3291) (#3306) * wip: sharded digest (probe base) * fix(scripts,tests): shard the module-size digest by scope, both halves One repo-wide ALLOWLIST_DIGEST made every open PR touching ANY budget conflict with every other one: they all rewrote the same pinned line, whatever they changed. The pin is now one entry per packages/<name> / apps/<name> / rust/<crate>, so PRs in different scopes edit different lines and git merges them (#3291). MEASURED BOTH WAYS, with the same two-branch probe. Two branches off one base, each raising one budget in a different scope and re-pinning: old single digest merge exit 1, CONFLICT (content) in check-module-size.mjs sharded merge exit 0, 0 conflicts The first run of that probe was wrong and I nearly kept it: the second branch's sed had not matched, so it committed nothing and the merge succeeded trivially. A clean merge of an empty branch proves nothing. The numbers above are from the redo, where both branches change 2 files each. HOW MUCH IT ACTUALLY BUYS, on the incident the issue measured. The four PRs touched: #3239 {apps/viewer, packages/export}, #3243 {packages/parser}, #3262 {packages/drawing-2d, packages/parser}, #3264 {apps/viewer}. Of the six pairs, four become independent and two still collide -- #3239/#3264 share apps/viewer, #3243/#3262 share packages/parser. So this removes CROSS-scope coupling, not within-scope, and both allowlists are concentrated: 48% of TS rows are apps/viewer, 61% of Rust rows are rust/geometry. Worth stating plainly rather than claiming the class is dead. BOTH HALVES, and the Rust one is the bigger problem. I assumed TS was the painful side because that is where last night's batch landed. Counting commits in the last 200 on main: rust/processing/tests/module_size_allowlist.txt 69, scripts/module-size-allowlist.txt 8. Deferring the Rust half would have deferred 8.6x the contention, and leaving one twin sharded and the other not is the drift I flagged on vercel-install.sh earlier today. The TS/Rust parity test now compares the two SCOPE TABLES rather than two numbers, with an anti-vacuity guard: a regex that matched nothing would have given two empty maps and a passing deepEqual. Verified: 796 JS tests, 5 Rust ratchet tests, clippy -D warnings clean. Mutations, each reddening only what it should -- collapsing every scope to one bucket (4 JS tests), disabling the drift check (2), perturbing one Rust scope pin (names that scope alone), drifting a Rust pin against the JS side (parity). An orphaned pin -- a scope whose rows all vanished -- is drift too, on both sides, or deleting a scope leaves a pin describing nothing and the gate stays silent. `cargo fmt` on the Rust file also reformatted two hunks I never touched; those are reverted, per the repo's no-blanket-fmt rule. * fix(scripts,tests): close the scope-rule gap /simplify found, and retract a bad measurement THE MEASUREMENT I PUT IN A CODE COMMENT WAS WRONG, and the review caught it. I wrote that this Rust allowlist is touched "69 of the last 200 commits on main against 8" for the TS one, and used that to argue the Rust half was the busier side. `git log -200 -- <path>` limits to 200 commits TOUCHING that path, so it returned each file's WHOLE history -- and the TS file was three days old while the Rust one was seven weeks. Counting properly, of the last 200 commits on main: 6 touch the Rust allowlist, 18 touch the TS one. The TS side is busier by 3x, the opposite of what I claimed. Sharding both halves is still right, for the reason that always held: two twin gates where one is sharded and the other is not is the drift I flagged on vercel-install.sh this morning. The comment now says that instead, and records the miscount so the next reader does not repeat it. THE SCOPE RULES COULD DIVERGE SILENTLY. `allowlistScope` (JS) and `allowlist_scope` (Rust) are the same rule in two languages, and the digest parity test could not see a divergence: it runs the shared rule only over the RUST allowlist, which holds ZERO packages/ rows, so that branch is dead on its only input. Measured -- deleting `"packages"` from the Rust rule left every digest byte-identical and every gate green. Closed with the pattern this repo already uses for csv_cell_vectors.json and unit_scale_vectors.json: a shared JSON fixture both sides read, carrying the paths production data does not contain. That mutation now fails on the Rust side, and the mirror mutation fails on the JS side. The fixture also surfaced a divergence that already existed: for an empty or leading-slash path JS returned "other" and Rust returned "". Rust's fallback was unreachable (`str::split` never yields an empty iterator). Reconciled on "other". SEVEN MORE, each verified by running it: - the failure's own remedy was impossible: "empty it and run this script to read the true values" lands on the guard that rejects an empty object and prints nothing else. Now names a placeholder entry, which does print all 37. - an orphan-only failure said "0 scope(s) disagree" above a list of orphans. Counted, and now tested -- that branch cites #3200 and nothing checked it fired. - `selfText.replace(PIN_RE, nextBlock)` treated `$&` in a scope name as a substitution pattern. Replacer function instead. - the Rust doc omitted `packages/<name>` while the code matched it -- exactly the edit that produces the silent divergence above. - `parse_allowlist()` ran three times per Rust check; once now. - the depth-3 justification was wrong. Three levels is not "too fine", it is a NO-OP: segment 3 is `src` for 307 of 309 TS rows and 65 of 65 Rust rows. - an unverifiable session anecdote ("eight PRs, 18 conflict resolutions") was repeated in four files. Replaced with the checkable numbers: of the batch's six PR pairs, four become independent and two still collide on shared scopes. 798 JS tests, 6 Rust ratchet tests, clippy clean, and my own added lines are rustfmt-clean (the two remaining diffs at :239 and :303 are pre-existing on main, confirmed by content match, and left alone). * fix(scripts,tests): state the real residual — adjacent pin lines still conflict /code-review found the justification claiming more than was measured, for the second time on this PR. The first was the contention miscount; this is the merge-conflict claim. I wrote that the only remaining collision is two PRs in the SAME scope, and called that "the honest limit of this change". It is not the limit. Git cannot auto-merge two edits on ADJACENT lines, so two PRs in DIFFERENT scopes whose pin entries happen to sit next to each other still conflict. Verified with the same two-branch probe, one budget raised per side, each side confirmed to have changed exactly one line before merging: old scheme, any two scopes CONFLICT same scope CONFLICT adjacent pin lines CONFLICT <- the case I missed two or more lines apart clean Measured: 36 of the 666 cross-scope pairs in the 37-entry TS table (5.4%), and 5 of 15 (33%) in the 6-entry Rust table. The Rust half is worse for a reason unrelated to how often it changes -- a smaller table makes adjacency likelier -- which belongs next to the twin-drift argument rather than leaving the impression that sharding is uniformly effective. Sharding still takes both from 100%, and the remedy is one line either way. But "PRs in other scopes are unaffected", which the allowlist header claimed, is false, and all three copies now say what was measured. Also dropped the now-dead `allowlistDigest` import, which this diff orphaned when it replaced both call sites -- `check-lint-ran` was reporting it as a new no-unused-vars warning. Everything else in that review came back clean, including the five ways the ratchet could have been loosened silently, the group/hash injectivity, the include_str!/readFileSync pair reading the same bytes, every degenerate --digests input failing closed, and --update reproducing both committed files byte-for-byte. 798 JS tests, 6 Rust tests, clippy clean, gates green.
2 participants
Refs #3260
ifcTypeCriteriondefaultsincludeSubtypestotrue, so a graphic-override rule naming a supertype is meant to style everything beneath it. The expansion ran offIFC_TYPE_HIERARCHY, a hand-written table inrule-engine.ts. Derived against the IFC4 (ADD2 TC1)SCHEMA_REGISTRYparent chain:IfcBuildingElementIfcDistributionElementIfcDistributionElementresolvedIfcDistributionFlowElementandIfcDistributionControlElementand stopped, because neither was itself a key — so no duct, pipe, cable, terminal, valve or sensor was ever styled. The elements still drew; they drew without the override the rule asked for, with no warning.Two further defects in the same table:
IfcFlowElementis not an IFC entity. It appeared in exactly one file in the repo — this table. Absent from IFC2X3 (ENTITIES_IFC2X3), IFC4 and IFC4X3, so it was never a legacy alias.IfcStairFlightunderIfcStair,IfcRampFlightunderIfcRamp. IFC4 makes both flights siblings underIfcBuildingElement.Is the table curated policy, or drift?
Checked before widening. It is drift:
ifcTypeCriterion(['IfcDuctSegment', 'IfcDuctFitting'])), including MEP, which would be the obvious beneficiary of a supertype. So the table's only consumer is user-authored rules and the publicdrawing.ifcTypeCriterionSDK surface — where breadth is the whole point.IfcFlowElement, a name in none of the three bundled schemas.IfcBuildingElementlistsIfcWall,IfcSlab,IfcBeam,IfcColumn,IfcDoor,IfcWindowbut notIfcCurtainWallorIfcPlate. There is no performance or authoring story under which those eleven are in andIfcCurtainWallis out.The fix
The table moves to
ifc-type-hierarchy.tsas the direct-children map of every entity underIfcElementandIfcSpatialElement, derived from IFC4 ADD2 TC1 — 31 rows covering 146 entities. It had to move:rule-engine.tssat at exactly its recorded module-size budget (561), so the table could not grow in place. Its row drops 561 → 523;ALLOWLIST_DIGESTis re-pinned in the same commit. No new allowlist row — the new module is 158 lines.Rather than silently delete the dead key,
IfcFlowElementis kept in an explicitAUTHORING_ALIASESmap pointing atIfcDistributionFlowElement, the real supertype of the four names it used to list; a rule already naming it keeps working and now reaches the whole flow subtree. The two non-schema stair/ramp edges join it there — narrowing those rules would be its own silent regression. Keeping aliases in a separate map is what lets the parity test hold the derived map to the schema exactly.getIfcSubtypesnow de-duplicates and tracks visited nodes, so an alias pointing back into the table cannot spin. The old version recursed unguarded.Deriving from
SCHEMA_REGISTRYat module load was the obvious alternative and is what several packages do, butdrawing-2ddepends only on@ifc-lite/geometry; making a published drawing package pull the 63k-line parser registry at runtime to answer a subtype question is a poor trade. The literal table stays, and a test — not a comment — holds it to the schema.Tests
The module had none. Two suites:
rule-engine.test.tsdrives rules through the real path (createOverrideEngine().applyOverrides) against named required entities per supertype, not a count floor — a count reds on benign schema growth and stays silent when a name drops out. Includes a positive anti-vacuity control and negative controls (IfcBuildingElementmust not reachIfcSpace;IfcWallmust not reachIfcSlab) so the matcher cannot pass by matching everything.ifc-type-hierarchy.test.tsre-derives the hierarchy from@ifc-lite/data'sENTITIES_IFC4— deliberately a different authority from the parser registry the table was generated from, so the two cross-check — and fails if the table omits a schema subtype, lists a name that is not an IFC4 entity, claims an edge the schema lacks, or grows a key outside the covered roots. Guarded against vacuity by asserting the derived universe is non-trivial before any subset check.@ifc-lite/datais a devDependency; the published bundle is unchanged.RED on
upstream/main:Tests 33 failed | 467 passed (500).GREEN:
Tests 511 passed (511);@ifc-lite/sdkalso green (188 passed).The parity guard was verified to fire rather than assumed: deleting
IfcCurtainWallfrom the table reds with"IfcBuildingElement does not reach: IfcCurtainWall"and"IfcElement does not reach: IfcCurtainWall". The table was then restored by the inverse edit and proved byte-identical withdiff.🤖 Generated with Claude Code