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

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

Author: BIMvoice

apps/viewer/src/store/globalId.test.ts

@@ -23,8 +23,12 @@ import {
   toGlobalIdFromModels,
   fromGlobalIdFromModels,
   toGlobalIdForRef,
+  localIdInParseRange,
+  localIdInOverlay,
   type ForwardModelMapLike,
+  type OwnershipView,
 } from './globalId.js';
+import { modelRemovedScope } from './teardown-scope.js';
 
 type ReverseEntry = Pick<FederatedModel, 'idOffset' | 'maxExpressId'>;
 
@@ -138,3 +142,97 @@ describe('fromGlobalIdFromModels', () => {
     }
   });
 });
+
+/**
+ * `localIdInParseRange` / `localIdInOverlay` — the "does a surviving model
+ * own this global id" rule (#3343). Exported here so `modelSlice.ts`'s
+ * resolvers and `teardown-scope.ts`'s `modelRemovedScope` survivor check
+ * share ONE implementation instead of three hand-written copies (the third,
+ * `fromGlobalIdFromModels` above, stays a deliberate fourth spelling — see
+ * its own boundary comment).
+ */
+describe('localIdInParseRange', () => {
+  it('returns the local id inside [idOffset, idOffset + maxExpressId], both boundaries included', () => {
+    const model = { idOffset: 1000, maxExpressId: 300 };
+    assert.equal(localIdInParseRange(model, 1000), 0);
+    assert.equal(localIdInParseRange(model, 1300), 300);
+    assert.equal(localIdInParseRange(model, 1150), 150);
+  });
+
+  it('returns null one past either boundary', () => {
+    const model = { idOffset: 1000, maxExpressId: 300 };
+    assert.equal(localIdInParseRange(model, 999), null);
+    assert.equal(localIdInParseRange(model, 1301), null);
+  });
+});
+
+describe('localIdInOverlay', () => {
+  it('returns null with no mutation view', () => {
+    const model = { idOffset: 0, maxExpressId: 100 };
+    assert.equal(localIdInOverlay(model, 150, undefined), null);
+  });
+
+  it('returns null for an id inside the parse range — not overlay\'s business', () => {
+    const model = { idOffset: 0, maxExpressId: 100 };
+    const view: OwnershipView = { getNewEntity: () => ({}) };
+    assert.equal(localIdInOverlay(model, 50, view), null);
+  });
+
+  it('returns the local id when the overlay view holds an entity above maxExpressId', () => {
+    const model = { idOffset: 0, maxExpressId: 100 };
+    const view: OwnershipView = { getNewEntity: (id) => (id === 150 ? {} : null) };
+    assert.equal(localIdInOverlay(model, 150, view), 150);
+  });
+
+  it('returns null when the overlay view has nothing at that local id', () => {
+    const model = { idOffset: 0, maxExpressId: 100 };
+    const view: OwnershipView = { getNewEntity: () => null };
+    assert.equal(localIdInOverlay(model, 150, view), null);
+  });
+});
+
+/**
+ * The scenario `modelSlice.ts`'s `resolveGlobalIdFromModels` doc-comment
+ * warns about: model A's overlay-allocated ids can land inside model B's
+ * PARSE-time range, because overlay ids simply increment past A's
+ * `maxExpressId` with no knowledge of where B starts. `resolveGlobalIdFromModels`
+ * handles this with two full passes (every model's parse range, THEN every
+ * model's overlay) specifically so a real, parsed entity in B always wins
+ * identity resolution over a synthetic overlay id in A.
+ *
+ * `modelRemovedScope`'s survivor check only ever needs "does SOME survivor
+ * own this id", not "which one" — and that boolean is the same regardless of
+ * which model or which check (parse range vs. overlay) is tried first, since
+ * it is a plain OR across survivors. This test pins that: it is what makes it
+ * safe for `modelRemovedScope` to check each survivor's parse range THEN
+ * overlay before moving to the next, rather than mirroring the two full
+ * passes `resolveGlobalIdFromModels` needs for identity.
+ */
+describe('parse-range vs. overlay ownership — cross-model shadowing', () => {
+  it('a survivor\'s PARSE-range id is not mistaken for stale even when an earlier survivor\'s overlay could also claim it', () => {
+    // A: parse range [0, 100], overlay claims local id 150 (global 150).
+    // B: parse range [101, 200] — globalId 150 falls in B's parse range too
+    // (150 - 101 = 49, inside [0, 100]).
+    const modelA = { id: 'A', idOffset: 0, maxExpressId: 100 };
+    const modelB = { id: 'B', idOffset: 101, maxExpressId: 100 };
+    const overlayA: OwnershipView = { getNewEntity: (id) => (id === 150 ? {} : null) };
+
+    // Both models claim globalId 150 by DIFFERENT rules — A via overlay, B
+    // via its own parse range. `modelRemovedScope` only needs to know it is
+    // owned by someone, and does not care which; it must not report this id
+    // stale regardless of survivor iteration order.
+    const state = {
+      models: new Map([
+        ['A', modelA],
+        ['B', modelB],
+      ]),
+      mutationViews: new Map([['A', overlayA]]),
+    } as Parameters<typeof modelRemovedScope>[0];
+
+    const scope = modelRemovedScope(state, 'unrelated-removed-model');
+    assert.equal(scope.isStale(150), false, 'globalId 150 is owned (by B\'s parse range, at least) — must not be purged');
+
+    // And B's own answer for that id is unambiguous, independent of A's overlay.
+    assert.equal(localIdInParseRange(modelB, 150), 49);
+  });
+});

apps/viewer/src/store/globalId.ts

@@ -82,3 +82,49 @@ export function toGlobalIdForRef(
 ): number {
   return toGlobalIdFromModels(models, ref.modelId, ref.expressId);
 }
+
+/** The model shape both `localIdInParseRange` and `localIdInOverlay` need. */
+export type OwnershipModel = Pick<FederatedModel, 'idOffset' | 'maxExpressId'>;
+
+/** The mutation-view shape `localIdInOverlay` needs — just enough to ask "does an overlay entity live at this local id". */
+export interface OwnershipView {
+  getNewEntity(id: number): unknown;
+}
+
+/**
+ * Parse-time ownership: a model owns `[idOffset, idOffset + maxExpressId]` from
+ * the original parse. Returns the LOCAL express id, or `null`.
+ *
+ * `model.idOffset` bare, no `?? 0`: it is a required `number` on
+ * `FederatedModel` (`store/types.ts`), and every caller of this has always
+ * read it bare. `null` is returned for a miss, so a caller must test
+ * `!== null` — local id `0` is a legitimate answer and a truthiness test
+ * would drop it.
+ *
+ * The single shared home for this rule (#3343): `modelSlice.ts`'s unscoped
+ * and scoped resolvers, and `teardown-scope.ts`'s `modelRemovedScope` survivor
+ * check, all call this instead of re-spelling the range arithmetic. Before
+ * the consolidation the three copies had already drifted once (#2697) and
+ * were independently re-merged by an unrelated teardown refactor (#3358) —
+ * nothing structurally stopped them drifting again.
+ */
+export function localIdInParseRange(model: OwnershipModel, globalId: number): number | null {
+  const localId = globalId - model.idOffset;
+  return localId >= 0 && localId <= model.maxExpressId ? localId : null;
+}
+
+/**
+ * Overlay ownership: duplicates / scripted adds through StoreEditor land ABOVE
+ * the model's parse-time `maxExpressId`, so `localIdInParseRange` cannot see
+ * them; the model's mutation view can. Returns the LOCAL express id, or `null`.
+ */
+export function localIdInOverlay(
+  model: OwnershipModel,
+  globalId: number,
+  view: OwnershipView | undefined,
+): number | null {
+  if (!view) return null;
+  const localId = globalId - model.idOffset;
+  if (localId <= model.maxExpressId) return null; // parse-range's business
+  return view.getNewEntity(localId) !== null ? localId : null;
+}

apps/viewer/src/store/slices/modelSlice.ts

@@ -15,6 +15,7 @@ import type { StateCreator } from 'zustand';
 import type { FederatedModel } from '../types.js';
 import { federationRegistry, type GlobalIdLookup } from '@ifc-lite/renderer';
 import type { ViewerState } from '../index.js';
+import { localIdInParseRange, localIdInOverlay } from '../globalId.js';
 import { viewerTeardown } from '../teardown-registry.js';
 import { modelRemovedScope } from '../teardown-scope.js';
 import {
@@ -96,7 +97,9 @@ export interface ModelSlice {
    * It shares the range and overlay predicates with the unscoped resolver
    * above, so the two cannot drift — a private range check in a caller is how
    * this codebase produced two resolvers that disagreed about the same id space
-   * (#2697).
+   * (#2697). Those predicates (`localIdInParseRange` / `localIdInOverlay`) live
+   * in `store/globalId.ts`, the same functions `teardown-scope.ts`'s
+   * `modelRemovedScope` calls for its survivor check (#3343).
    *
    * Not the only spelling in the repo, and this doc must not claim otherwise:
    * `store/globalId.ts` `fromGlobalIdFromModels` holds an independent copy that
@@ -110,35 +113,12 @@ export interface ModelSlice {
 }
 
 /**
- * Parse-time ownership: a model owns `[idOffset, idOffset + maxExpressId]` from
- * the original parse. Returns the LOCAL express id, or `null`.
- *
- * `model.idOffset` bare, no `?? 0`: it is a required `number` on
- * `FederatedModel` (`store/types.ts`), and the unscoped resolver this is
- * extracted from has always read it bare. `null` is returned for a miss, so a
- * caller must test `!== null` — local id `0` is a legitimate answer and a
- * truthiness test would drop it.
- */
-function localIdInParseRange(model: FederatedModel, globalId: number): number | null {
-  const localId = globalId - model.idOffset;
-  return localId >= 0 && localId <= model.maxExpressId ? localId : null;
-}
-
-/**
- * Overlay ownership: duplicates / scripted adds through StoreEditor land ABOVE
- * the model's parse-time `maxExpressId`, so `localIdInParseRange` cannot see
- * them; the model's mutation view can. Returns the LOCAL express id, or `null`.
+ * `localIdInParseRange` / `localIdInOverlay` live in `store/globalId.ts` now
+ * (#3343) — that is the cycle-free home for the "does a surviving model own
+ * this global id" rule shared with `teardown-scope.ts`'s `modelRemovedScope`.
+ * They used to be defined here; keep this pointer so a reader who remembers
+ * that lands in the right file.
  */
-function localIdInOverlay(
-  model: FederatedModel,
-  globalId: number,
-  view: { getNewEntity: (id: number) => unknown } | undefined,
-): number | null {
-  if (!view) return null;
-  const localId = globalId - model.idOffset;
-  if (localId <= model.maxExpressId) return null; // parse-range's business
-  return view.getNewEntity(localId) !== null ? localId : null;
-}
 
 /** The mutation views registered on the store, if the owning slice is present. */
 function mutationViewsOf(

apps/viewer/src/store/teardown-scope.ts

@@ -19,26 +19,33 @@
  */
 
 import type { TeardownScope, TeardownState } from './teardown.js';
+import { localIdInParseRange, localIdInOverlay } from './globalId.js';
 
 /** The `model-removed` arm, once its predicate is known. */
 export type ModelRemovedScope = Extract<TeardownScope, { kind: 'model-removed' }>;
 
 /**
  * Build the scope for "this model is going away".
  *
- * KNOWN DUPLICATION, deliberately left: the survivor predicate below is a third
- * statement of the ownership rule that `modelSlice`'s `localIdInParseRange` /
- * `localIdInOverlay` (#2697) and `store/globalId.ts` already carry. It cannot
- * import `modelSlice` — that file imports this one — and `store/globalId.ts` is
- * the cycle-free home all three should share. Consolidating them is a change of
- * its own; until then, a boundary change has to be made in three places.
+ * The survivor predicate below calls `localIdInParseRange` / `localIdInOverlay`
+ * (`store/globalId.ts`) — the same functions `modelSlice`'s unscoped and
+ * scoped resolvers call (#2697) — instead of re-spelling the range/overlay
+ * arithmetic a third time (#3343). `store/globalId.ts` is the cycle-free home:
+ * this file cannot import `modelSlice` (that file imports this one), and
+ * `globalId.ts` imports neither.
  *
  * The predicate mirrors the two-pass resolution in `modelSlice`'s
  * `resolveGlobalIdFromModels`: a global id survives if some SURVIVING model
  * owns it, either inside its parse-time range (`idOffset` ..
  * `idOffset + maxExpressId`) or as an overlay-allocated entity above that
  * range in its mutation view (StoreEditor duplicates, scripted adds). An id no
- * survivor owns is stale, and every global-id-keyed slice drops it.
+ * survivor owns is stale, and every global-id-keyed slice drops it. Unlike
+ * `resolveGlobalIdFromModels`'s two full passes (parse range for every model,
+ * THEN overlay for every model — needed there because it must pick a single
+ * WINNING model), this only needs a yes/no per id, so it checks each survivor
+ * fully (parse range, then overlay) before moving to the next: the two orders
+ * agree on membership because "some survivor owns it via A or B" is the same
+ * boolean regardless of which survivor or which check is tried first.
  *
  * @param state - the store as it stands BEFORE the teardown's `set`. Read-only
  *   and partial: `slices/modelSlice.test.ts` drives `removeModel` through a
@@ -69,10 +76,8 @@ export function modelRemovedScope(
 
   const isStale = (id: number): boolean => {
     for (const survivor of survivors) {
-      const localId = id - survivor.idOffset;
-      if (localId < 0) continue;
-      if (localId <= survivor.maxExpressId) return false;
-      if (mutationViews?.get(survivor.id)?.getNewEntity(localId) != null) return false;
+      if (localIdInParseRange(survivor, id) !== null) return false;
+      if (localIdInOverlay(survivor, id, mutationViews?.get(survivor.id)) !== null) return false;
     }
     return true;
   };