fix(export): derive rooted-type detection from the schema, matching the Rust side by BIMvoice · Pull Request #3030 · LTplus-AG/ifc-lite

BIMvoice · 2026-08-21T13:51:35Z

Replaces the JS hand-maintained "non-rooted types" denylist with getInheritanceChainAcrossSchemas(type).includes('IfcRoot') — the schema-derived positive check.

This closes the divergence flagged on #3007. That PR moved the Rust exporter to a schema-derived allowlist (IfcType::is_subtype_of(IfcRoot)) while JS kept a hand-kept denylist, so the two exporters could disagree about which entities get GUID-reconciled. After this, both answer the question from the same schema rather than from two lists someone has to keep in step.

RED — surgical, reverting the call site and restoring the deleted 41-entry denylist constant, so it runs against the genuine old behaviour rather than a stub:

× does not mistake a 22-char material Name for a GlobalId,
  while still reconciling a real rooted collision
1 failed / 770 passed / 30 skipped

That test is two-way — it pins the exclusion and that real rooted collisions still reconcile. 771 pass after. Typecheck and all three lint gates clean.

A suspected "guard that cannot detect what it claims", chased and cleared

The old code upper-cased (ref.type.toUpperCase()); the new one passes raw ref.type, while STEP types are uppercase and the chain returns PascalCase IfcRoot. Had the lookup been case-sensitive, every type would read as non-rooted and GlobalId reconciliation would have silently stopped repo-wide — and the changeset's own "safe-miss direction" reasoning would have masked it.

Ran it rather than reasoning about it. The function normalises internally: IFCWALL, IFCDOOR, IFCPROPERTYSET, IFCTASK, IFCGROUP, IFCWALLSTANDARDCASE → rooted; IFCPROPERTYSINGLEVALUE, IFCMATERIAL, IFCPERSON, IFCTEXTLITERAL, IFCCLASSIFICATION, IFCQUANTITYLENGTH → not rooted. All correct.

The changeset's headline claim also checks out: IFCMATERIALPROFILEWITHOFFSETS reads non-rooted and was genuinely absent from the old denylist.

Disclosed, not a defect: a vendor-extension type subclassing IfcRoot yields an empty chain (IFCVENDORFOO → len=0) and is now treated non-rooted, losing a reconciliation it previously got. The changeset states this direction explicitly, and it is the safe one.

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Improved merged STEP export handling for entities with names resembling GlobalIds.
- Prevented non-rooted entities from being incorrectly unified or assigned new identifiers.
- Preserved correct reconciliation of shared GlobalIds across rooted entities.
Tests
- Added regression coverage for material profiles, distinct offsets, unified walls, and dangling references.

… denylist with a schema check extractGlobalIdFast identified a rooted entity's GlobalId positionally, gated by NON_ROOTED_STRING_TYPES — a hand-maintained denylist of non-rooted types that lead with a Name/Identifier string. The list was incomplete (IfcMaterialProfileWithOffsets and other resource types were missing), so a merge could misread such an entity's Name as a GlobalId and, on a coincidental collision, silently unify it away — corrupting ordinary model data. Replaced the denylist with a schema-derived positive check, getInheritanceChainAcrossSchemas(type).includes('IfcRoot'), mirroring the Rust exporter's IfcType::is_subtype_of(IfcRoot) so the two sides agree on what "rooted" means instead of keeping separate hand-maintained answers.

coderabbitai · 2026-08-21T13:52:35Z

Warning

Review limit reached

@louistrue, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 54 minutes

Limit details: You’ve used all 2 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 189cf061-aca5-408a-90a8-7fdf914ea2c4

📥 Commits

Reviewing files that changed from the base of the PR and between fe38b33 and f780399.

📒 Files selected for processing (3)

.changeset/js-guid-denylist.md
packages/export/src/merged-exporter.test.ts
packages/export/src/merged-exporter.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: af3f1548-ee13-4d79-a5b5-e513671cb34f

📥 Commits

Reviewing files that changed from the base of the PR and between 9279987 and 3758817.

📒 Files selected for processing (3)

.changeset/js-guid-denylist.md
packages/export/src/merged-exporter.test.ts
packages/export/src/merged-exporter.ts

Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.

📝 Walkthrough

Walkthrough

The merged STEP exporter now uses IFC schema inheritance to identify IfcRoot subtypes before extracting positional GlobalIds. Regression coverage verifies distinct material profiles remain separate while walls with shared GlobalIds reconcile correctly.

Changes

Merged export rooted-entity detection

Layer / File(s)	Summary
Schema-derived rooted-entity classifier `packages/export/src/merged-exporter.ts`	The exporter replaces the manually maintained non-rooted type denylist with an `IfcRoot` inheritance check. Unknown types are treated as non-rooted.
GlobalId extraction and regression coverage `packages/export/src/merged-exporter.ts`, `packages/export/src/merged-exporter.test.ts`, `.changeset/js-guid-denylist.md`	GlobalId extraction uses the schema-derived classifier. Tests cover non-rooted material names, shared wall GlobalIds, distinct offsets, and complete references. The changeset records the patch release.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 37588

The exporter now derives rooted-type detection from the schema, aligning GUID reconciliation with the Rust implementation, with regression coverage for rooted collisions and material-name handling. No actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: louistrue

Poem

A rabbit checks the roots below,
Where schema paths now clearly show.
Walls unite by matching sign,
Materials keep their own design.
Clean references hop in line!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: schema-derived rooted-type detection in the export path, aligned with the Rust implementation.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.)
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-08-21T13:56:11Z

Viewer benchmark

✅ No threshold regressions detected.

01_Snowdon_Towers_Sample_Structural(1).ifc

Baseline recorded 2026-07-01T20:31:05.538Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.

Metric	Current	Baseline	Delta	Threshold	Status
firstBatchWaitMs	1662ms	2905ms	-42.8%	+50%	✅
firstVisibleGeometryMs	2478ms	3652ms	-32.1%	+50%	✅
streamCompleteMs	2821ms	3598ms	-21.6%	+50%	✅
spatialReadyMs	1246ms	1032ms	+20.7%	+50%	✅
metadataCompleteMs	1781ms	3063ms	-41.9%	+50%	✅
totalWallClockMs	2900ms	3700ms	-21.6%	+50%	✅

AC20-FZK-Haus.ifc

Baseline recorded 2026-07-01T20:30:59.972Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.

Metric	Current	Baseline	Delta	Threshold	Status
firstBatchWaitMs	368ms	1075ms	-65.8%	+50%	✅
firstVisibleGeometryMs	1435ms	1572ms	-8.7%	+50%	✅
streamCompleteMs	1012ms	1980ms	-48.9%	+50%	✅
spatialReadyMs	1051ms	915ms	+14.9%	+50%	✅
metadataCompleteMs	1125ms	1392ms	-19.2%	+50%	✅
totalWallClockMs	1600ms	3300ms	-51.5%	+50%	✅

Refresh the baseline from a CI run: dispatch the Benchmark workflow with record_baseline, download the benchmark-baseline artifact, and commit baseline.json (see tests/benchmark/README.md).

vercel · 2026-08-21T21:08:14Z

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

Project	Deployment	Actions	Updated (UTC)
ifc-lite-dev	Ready	Preview	Aug 21, 2026 9:11pm

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
ifc-lite-viewer-embed	Ignored		Aug 21, 2026 9:11pm

louistrue · 2026-08-21T22:13:38Z

Merge review: approved. Schema-derived rootedness replaces the hand-maintained denylist correctly.

getInheritanceChainAcrossSchemas normalizes case internally, so the parity claim checks out against packages/parser/src/ifc-schema.ts:113.

Three mutations run, three killed, rather than reasoned about:

old 41-entry denylist restored at the call site: only the new test red (47/48)
isRootedType hard-coded false: 5 tests red, so the reconciliation path is protected
hard-coded true: 3 tests red, so the exclusion path is protected too

That last pair matters: a one-directional assertion would be satisfied by a mutation that hard-codes the answer, and both directions failing is what rules that out. The pre-existing property and text-literal tests now exercise the schema path as well.

Changeset present. 18 checks pass, 0 fail, 0 cancelled, on the current CI lane.

Non-blocking note, recorded rather than acted on: isRootedType walks the inheritance chain per entity reference where the old code did an O(1) Set lookup. Nothing on this board makes that matter, but if merge performance ever shows up in a profile, a one-line memo Map recovers it.

BIMvoice · 2026-08-22T05:04:03Z

@coderabbitai review

Requesting the review that never ran: the check on this PR reports pass, but the bot's only comment here is the Fair Usage "Review limit reached" notice, so no code was looked at. Spending the reset quota on this one first because it is the closest to merge — @louistrue has already approved it by hand.

coderabbitai · 2026-08-22T05:04:10Z

Your plan includes PR reviews subject to rate limits. More reviews will be available in 44 minutes.

BIMvoice requested a review from louistrue as a code owner August 21, 2026 13:51

BIMvoice mentioned this pull request Aug 21, 2026

Rust and JS exporters classify rooted entities differently, and their GUID minting is unpinned #3015

Open

Merge branch 'main' into fix-js-guid-denylist

f780399

vercel Bot deployed to Preview – ifc-lite-dev August 21, 2026 21:11 View deployment

louistrue merged commit 0146f0a into main Aug 21, 2026
24 checks passed

Navigation Menu

Uh oh!

Uh oh!

fix(export): derive rooted-type detection from the schema, matching the Rust side - #3030

fix(export): derive rooted-type detection from the schema, matching the Rust side#3030
louistrue merged 2 commits into
mainfrom
fix-js-guid-denylist

BIMvoice commented Aug 21, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 21, 2026 •

edited

Loading

Review limit reached

Walkthrough

Changes

Uh oh!

github-actions Bot commented Aug 21, 2026 •

edited

Loading

Uh oh!

vercel Bot commented Aug 21, 2026 •

edited

Loading

Uh oh!

louistrue commented Aug 21, 2026

Uh oh!

Uh oh!

BIMvoice commented Aug 22, 2026

Uh oh!

coderabbitai Bot commented Aug 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

BIMvoice commented Aug 21, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

A suspected "guard that cannot detect what it claims", chased and cleared

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Uh oh!

github-actions Bot commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Viewer benchmark

01_Snowdon_Towers_Sample_Structural(1).ifc

AC20-FZK-Haus.ifc

Uh oh!

vercel Bot commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

louistrue commented Aug 21, 2026

Uh oh!

Uh oh!

BIMvoice commented Aug 22, 2026

Uh oh!

coderabbitai Bot commented Aug 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

BIMvoice commented Aug 21, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 21, 2026 •

edited

Loading

github-actions Bot commented Aug 21, 2026 •

edited

Loading

vercel Bot commented Aug 21, 2026 •

edited

Loading