fix(mutations): treat a base null-valued property as present, not absent

[BIMvoice]

Fixes MutablePropertyView.setProperty() misclassifying the first edit of an already-present-but-null base property as CREATE_PROPERTY instead of UPDATE_PROPERTY.

The body previously here described source-msgraph's page-limit and sign-out fixes — that is #3031's content, pasted into the wrong PR. This diff is three files, all under packages/mutations. Corrected.

The defect

propExistedBefore relied on oldValue !== null plus an in-session newPsets check. Neither sees a property that exists in the base property table with a null value — an unset Boolean parsed from the source IFC file, the same shape as #1107 — because getPropertyValue() legitimately returns null for it before any edit.

deleteProperty() already avoids this trap with its own propExistsInBase lookup against the base pset's property list. setProperty() now checks the same thing.

It matters because the mutation type is the contract: mutationSlice.ts:2693 decides undo by type — its own comment says so. Undoing a CREATE deletes the property; undoing an UPDATE replays oldValue.

The two-way half, added after an adversarial pass

The first version of this fix counted a base property as present even after the user had deleted it. Measured, before and after that disjunct:

sequence	pre-PR	first version of the fix
deleteProperty then setProperty on a base prop	CREATE_PROPERTY	UPDATE_PROPERTY, oldValue null
deletePropertySet then setProperty	CREATE_PROPERTY	UPDATE_PROPERTY, oldValue null

So after delete-then-re-set, undo would replay null and resurrect the deleted property as a present-but-unset row instead of removing it — exactly the failure this PR exists to prevent, reintroduced on the adjacent path.

The base-pset lookup now only counts a row that is still visible: a property the user has deleted, directly or by deleting its whole pset, is masked out of getForEntity(), so re-setting it stays a CREATE_PROPERTY.

Three tests: both delete shapes stay CREATE; an undeleted base property still classifies as UPDATE with its true oldValue, so the guard is not blanket.

Revert-verified rather than read: removing the !maskedInSession qualifier fails exactly the two delete tests and leaves the third green.

@ifc-lite/mutations 206 pass / 10 skip; typecheck green.

A pre-existing bug found while probing this, not fixed here

deletePropertySet(7,'Pset_Base') then setProperty(7,'Pset_Base','Status',…) leaves getForEntity(7) === [] — the write silently vanishes. psetExistsInBase is true so nothing goes into newPsets, and getForEntity skips the whole pset via deletedPsets (mutable-property-view.ts:314).

Reproduced on upstream/main, so it predates this PR and is out of scope here. Worth its own issue.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 25 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: 7525ddcd-2e15-42ed-8589-651d27e3259c

📥 Commits

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

📒 Files selected for processing (3)

• .changeset/mutations-set-property-base-null-classification.md
• packages/mutations/src/mutable-property-view.ts
• packages/mutations/test/mutable-property-view.test.ts

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

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	1835ms	2905ms	-36.8%	+50%	✅
firstVisibleGeometryMs	2641ms	3652ms	-27.7%	+50%	✅
streamCompleteMs	3119ms	3598ms	-13.3%	+50%	✅
spatialReadyMs	1377ms	1032ms	+33.4%	+50%	✅
metadataCompleteMs	1962ms	3063ms	-35.9%	+50%	✅
totalWallClockMs	3600ms	3700ms	-2.7%	+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	372ms	1075ms	-65.4%	+50%	✅
firstVisibleGeometryMs	1349ms	1572ms	-14.2%	+50%	✅
streamCompleteMs	1103ms	1980ms	-44.3%	+50%	✅
spatialReadyMs	1139ms	915ms	+24.5%	+50%	✅
metadataCompleteMs	1233ms	1392ms	-11.4%	+50%	✅
totalWallClockMs	1500ms	3300ms	-54.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]

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

2 Skipped Deployments

Project	Deployment	Actions	Updated (UTC)
ifc-lite-dev	Ignored	Preview	Aug 22, 2026 11:56am
ifc-lite-viewer-embed	Ignored		Aug 22, 2026 11:56am

[BIMvoice]

Self-review found a defect in this PR's own fix, now corrected — pushed d6267a8b3. Flagging it rather than quietly amending, because it is the shape this PR exists to prevent, reintroduced on the adjacent path.

The submitted version counted a base property as present even after the user had deleted it. Measured before and after the new disjunct:

sequence	pre-PR	as submitted
deleteProperty then setProperty on a base prop	CREATE_PROPERTY	UPDATE_PROPERTY, oldValue null
deletePropertySet then setProperty	CREATE_PROPERTY	UPDATE_PROPERTY, oldValue null

The consequence runs through undo: mutationSlice.ts:2693 decides undo by mutation type — its own comment says so — so undoing a CREATE deletes the property while undoing an UPDATE replays oldValue. After delete-then-re-set, undo would have replayed null and resurrected the deleted property as a present-but-unset row instead of removing it. That is the exact failure the changeset names as the reason for the fix.

The disjunct is now qualified with "not currently masked in this session" — a DELETE marker on the key, or the pset in deletedPsets. Three tests: both delete shapes stay CREATE, and an undeleted base property still classifies as UPDATE with its true oldValue, so the guard is not blanket. Revert-verified rather than read: removing the qualifier fails exactly the two delete tests and leaves the third green.

I have also rewritten the body. It described source-msgraph's page-limit and sign-out fixes — #3031's content, in the wrong PR. This diff is three files, all under packages/mutations. A reviewer reading top-down was reviewing a diff that is not here. My error, and the second time this week; I am checking bodies against gh pr diff --name-only before posting from now on.

One pre-existing bug found while probing, not fixed here: deletePropertySet(7,'Pset_Base') then setProperty(7,'Pset_Base','Status',…) leaves getForEntity(7) === [] — the write silently vanishes. psetExistsInBase is true so nothing enters newPsets, and getForEntity skips the whole pset via deletedPsets (mutable-property-view.ts:314). Reproduced on upstream/main, so it predates this PR. Happy to file it or fix it separately — say which you prefer.

206 pass / 10 skip, typecheck green.

[BIMvoice]

Fixture-symmetry sweep found this PR's own fixtures could not distinguish the two conditions its fix turns on. Fixed, pushed 92cec36bb.

Every base-pset fixture contained exactly the property being set, so "pset exists in base" and "property exists in base" coincided in all of them.

VERIFIED BY RUNNING: relaxing the new disjunct to basePsets.some(p => p.name === psetName) — dropping the inner property check entirely — left 28/28 green.

That relaxation is not harmless. It classifies every new property added to an existing pset as UPDATE with oldValue: null, so undo would leave a present-but-unset row rather than removing it — which is the exact failure this PR's changeset names as its reason for existing, arriving by a third route.

Fix: a pset holding IsExternal while LoadBearing is the property being set, so the two conditions come apart. The relaxation now fails.

[louistrue]

Reviewed this properly, since CodeRabbit was rate-limited on it and the green is therefore not a review. It holds up. One low-severity gap, no defects.

Verified by running, not reading

The new disjunct is a name membership test over the base pset's property array, so it is value-independent by construction. Probed every value shape against the real slice:

base value=null         -> UPDATE_PROPERTY  oldValue=null
base value=zero         -> UPDATE_PROPERTY  oldValue=0
base value=emptyString  -> UPDATE_PROPERTY  oldValue=
base value=false        -> UPDATE_PROPERTY  oldValue=false
base value=NaN          -> UPDATE_PROPERTY  oldValue=NaN
base ABSENT             -> CREATE_PROPERTY  oldValue=null

Present-with-null and absent are genuinely different states here, which is the thing this family of bug keeps getting wrong.

Worth stating explicitly because "does it also handle 0?" is the obvious next question and the answer is not "it was fixed too": 0, '', false and NaN were never broken. getPropertyValue returns them verbatim and oldValue !== null was already true for all of them. null was the only ambiguous value. So this is a complete fix, not a partial one that reads as complete.

No prototype exposure either: every lookup is Array.prototype.some with ===, Map.get, or Set.has — no bracket access on a plain object. Probed constructor, __proto__, toString and hasOwnProperty as both propName and psetName; all returned CREATE_PROPERTY.

The one finding, low severity

mutable-property-view.ts:476 — the deletedPsets half of maskedInSession is load-bearing and untested. Five clause-level mutants, four killed, this one survived:

M1 revert the whole base disjunct        killed
M2 drop !maskedInSession                 killed
M3 per-pset instead of per-property      killed
M5 drop the DELETE-marker half           killed
M4 drop the deletedPsets half            SURVIVED

It survives because deletePropertySet also writes a per-property DELETE marker, so the two halves overlap in every existing fixture. They stop overlapping once that marker is overwritten:

deletePropertySet(7,'Pset_Base'); setProperty(...,null); setProperty(...,null)
  with the clause    2nd call -> CREATE_PROPERTY   (correct, getForEntity(7) === [])
  without it         2nd call -> UPDATE_PROPERTY, oldValue=null

The clause is right and the code is right. There is simply no test that fails if someone deletes it, which is the untested vs redundant ambiguity — a surviving mutant is a question, not a verdict, and here the answer is "the only bound for one route". One test on that sequence closes it.

Positive controls are present and real: 'an undeleted base property is still an UPDATE (the guard is not blanket)' and the per-property test both fail if the classification is forced to always-present.

207 passed / 10 skipped, typecheck OK.

[BIMvoice]

Gap closed, pushed 64c7aeee6.

You were right that the deletedPsets half of maskedInSession survives mutation. Added a test on the deletePropertySet → setProperty(null) → setProperty(null) sequence.

Mutation-verified rather than assumed: dropping this.deletedPsets.has(...) from maskedInSession now fails exactly one test — the new one, with the other 29 green. Which also independently confirms your claim that M4 previously survived: before this, that mutation killed nothing.

That is the second half of the same two-way rule this PR is about. The first version guarded the property-level mask and left the pset-level one unpinned, so a regression that stopped honouring a deleted pset would have shipped green.