Skip to content

fix(viewer): collapse the global-id ownership rule to one owner - #3380

Open
BIMvoice wants to merge 1 commit into
mainfrom
fix-3343
Open

fix(viewer): collapse the global-id ownership rule to one owner#3380
BIMvoice wants to merge 1 commit into
mainfrom
fix-3343

Conversation

@BIMvoice

Copy link
Copy Markdown
Collaborator

Refs #3343.

What #3343 found, and what changed since it was filed

The issue named four copies of "does a surviving model own this global id" (parse-range, then overlay): modelSlice.ts's extracted localIdInParseRange / localIdInOverlay (#2697), modelSlice.ts's removeModel survivor loop (re-spelled by hand, not calling the extraction), syncSourceModel.ts's purgeStaleEntityState (a byte-identical second hand copy), and store/globalId.ts's fromGlobalIdFromModels (parse-range only, deliberately different — not a consolidation target).

That was accurate at the commit the issue was filed against. Since then, an unrelated refactor (#3358, already merged) replaced removeModel's and syncSourceModel's hand-rolled loops with one shared teardown-scope.ts's modelRemovedScope. Its own doc comment already flagged what was left: "KNOWN DUPLICATION, deliberately left: the survivor predicate below is a third statement of the ownership rule... store/globalId.ts is the cycle-free home all three should share. Consolidating them is a change of its own."

So on upstream/main today there are three implementations, not four: the extraction, the deliberately-different fromGlobalIdFromModels, and modelRemovedScope's copy.

Does the remaining pair actually disagree?

I checked before touching anything. modelRemovedScope's isStale only needs a boolean ("does some survivor own this id"), which is a plain OR across survivors of (parse-range match OR overlay match) — and OR is order-independent. resolveGlobalIdFromModels needs to pick which model owns an id, which is why it deliberately runs two full passes (every model's parse range, then every model's overlay) rather than checking each model's own overlay before the next model's parse range — its own comment explains why: an overlay id of the first model could shadow a plain parse-time id of the second. That hazard exists for identity resolution, not for the boolean isStale asks.

I fuzzed both formulations (300k randomized federated-model layouts, offsets, and overlay ids deliberately bled into a neighbor's parse range) and found zero disagreements between the old hand-rolled isStale and the shared functions, and zero disagreements between the old and new isStale implementations. So there is no live bug to reproduce as a failing test today — the two disagreeing copies the issue names were already unified by #3358. The remaining exposure was structural: nothing stopped the next edit to the rule from landing in one copy and not the other, exactly as the maintainer's own comment says.

What this PR does

Moves localIdInParseRange / localIdInOverlay into store/globalId.ts (the cycle-free home named in the comment: modelSlice.ts imports teardown-scope.ts, so the reverse would cycle; globalId.ts imports neither) and exports them. modelSlice.ts and teardown-scope.ts now both call the shared functions instead of re-deriving the range/overlay arithmetic. Behavior is unchanged — verified by the fuzz check above and by the full scoped test run below.

A fifth, ModelEntry-typed, parse-range-only spelling remains in apps/viewer/src/lib/lens/adapter.ts's resolveGlobalId (a result-object-reusing hot loop for lens evaluation, 100k+ calls). Left alone, per the issue's own direction — different module, different type, no overlay pass — and now more clearly justified: every caller of the shared functions works with Map entries, not a pooled result object reused across a hot loop.

Proof of RED

apps/viewer/src/store/globalId.test.ts gained direct tests for localIdInParseRange / localIdInOverlay, plus a cross-check that modelRemovedScope's isStale and the shared functions agree on the "overlay of model A lands inside model B's parse range" scenario modelSlice.ts's own comment warns about.

Proved RED by stashing only the three source files (globalId.ts, modelSlice.ts, teardown-scope.ts — keeping the new test), running the suite against the restored old code, confirming failure, then restoring by SHA (not stash pop):

SyntaxError: The requested module './globalId.js' does not provide an export named 'localIdInOverlay'
# fail 1

GREEN after restoring the fix: # tests 20 / # pass 20 / # fail 0.

Scoped regression run across every test file touching this rule (modelSlice, teardown-registry, teardown idempotence, model lifecycle x2, syncSourceModel, four removeModel-*-stale tests, clearAllModels-overlay-stale, lens adapter):

# tests 125
# suites 37
# pass 125
# fail 0

tsc --noEmit on apps/viewer: 0 errors. check-source-text-assertions.mjs: OK, 0 new. check-module-size.mjs: OK (both changed files shrink or stay well under budget; left the pre-existing unrelated allowlist headroom notes untouched to keep this PR scoped).

Overlap with in-flight viewer-store PRs

Checked #3367, #3371, #3372, #3375, #3379 (per the issue triage) — none touch modelSlice.ts, globalId.ts, or teardown-scope.ts. No file overlap.

No changeset: @ifc-lite/viewer is a private package and #3358 (the comparable teardown refactor this builds on) didn't carry one either.

Does not close #3343 — leaving that to the maintainer per policy.

`localIdInParseRange` / `localIdInOverlay` (modelSlice.ts, #2697) and
`teardown-scope.ts`'s `modelRemovedScope` survivor check re-spelled the
same "does a surviving model own this global id" rule by hand — the
latter's own comment already flagged this as known duplication that
"consolidating is a change of its own". Move the two predicates into
`store/globalId.ts`, the cycle-free home the comment already named, and
have both modelSlice.ts and teardown-scope.ts call them instead of
re-deriving the range/overlay arithmetic.

Issue #3343 described this as four copies, two of which ignored the
extraction and disagreed. That was accurate when filed, but an
unrelated teardown refactor (#3358, already merged) collapsed the two
disagreeing copies (modelSlice's removeModel and syncSourceModel's
purgeStaleEntityState) into the single teardown-scope.ts copy fixed
here. A 300k-case property check (isStale's old hand-rolled loop vs.
the shared functions, and identity resolution vs. staleness, across
random federated-model layouts with overlay ids bled into a
neighbor's parse range) found zero behavioral disagreement between
the current duplicate and the extraction — the boolean "some survivor
owns it" is a straight OR across survivors, so check order never
mattered for that question, only for the (unrelated, already-correct)
identity resolution the modelSlice comment about the two-pass order
already covers. The remaining risk was structural, not a live bug:
nothing stopped the next edit to the rule from being made in one
place and not the other.

`apps/viewer/src/lib/lens/adapter.ts` holds a fifth, parse-range-only
spelling in a result-object-reusing hot loop over its own `ModelEntry`
type; left alone as the issue asks (different module, no overlay
pass, doubly justified now since the shared function's callers all
resolve global ids into `Map` entries rather than reuse a pooled
object across a hot loop).

RED proven by stashing only the source changes (globalId.ts,
modelSlice.ts, teardown-scope.ts) and running the new test against
the restored old code: it failed to import `localIdInOverlay`
(SyntaxError: does not provide an export), then passed 20/20 after
restoring the fix.
@BIMvoice
BIMvoice requested a review from louistrue as a code owner August 28, 2026 06:46
@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Warning

Review limit reached

  • Run on-demand review

This review includes 4 billable files and costs up to $1.00.

Or wait 32 minutes for your next included review.

View limit details

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

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6525c66e-c14f-4840-ab78-c72671951807

📥 Commits

Reviewing files that changed from the base of the PR and between 5a431e5 and bcf986a.

📒 Files selected for processing (4)
  • apps/viewer/src/store/globalId.test.ts
  • apps/viewer/src/store/globalId.ts
  • apps/viewer/src/store/slices/modelSlice.ts
  • apps/viewer/src/store/teardown-scope.ts

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

@github-actions

Copy link
Copy Markdown
Contributor

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 1719ms 2905ms -40.8% +50%
firstVisibleGeometryMs 2362ms 3652ms -35.3% +50%
streamCompleteMs 2992ms 3598ms -16.8% +50%
spatialReadyMs 1184ms 1032ms +14.7% +50%
metadataCompleteMs 1742ms 3063ms -43.1% +50%
totalWallClockMs 3100ms 3700ms -16.2% +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 382ms 1075ms -64.5% +50%
firstVisibleGeometryMs 1342ms 1572ms -14.6% +50%
streamCompleteMs 1357ms 1980ms -31.5% +50%
spatialReadyMs 980ms 915ms +7.1% +50%
metadataCompleteMs 1090ms 1392ms -21.7% +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).

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.

viewer store: four copies of the global-id ownership rule, two ignoring the extraction

1 participant