Skip to content

fix(viewer): route SDK isolate() through the assembly-expansion resolver (#3338) - #3382

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

fix(viewer): route SDK isolate() through the assembly-expansion resolver (#3338)#3382
BIMvoice wants to merge 1 commit into
mainfrom
fix-3338

Conversation

@BIMvoice

Copy link
Copy Markdown
Collaborator

Summary

Refs #3338 ("Assembly drill-down: expansion is one call site every channel must remember to use"). This fixes ONE concrete, verified gap the issue's own enumeration missed; it does not attempt the larger open design question the issue raises (resolver-property vs explicit user action).

What's covered vs what forgets

expandToGeometryBearingIds (apps/viewer/src/utils/aggregation.ts) has one production call site, cameraCallbacks.resolveHighlightIds, defined in Viewport.tsx. Channels that route through it:

  • LensPanel.tsx:1403
  • PropertiesPanel.tsx:801
  • SearchModal.text.tsx:139
  • SearchModal.filter.tsx:401

A fifth channel, not named in #3338, did not: apps/viewer/src/sdk/adapters/visibility-adapter.ts's isolate() — reached by scripts (ifc.isolate(...)) and the MCP viewer_isolate tool. It already expands spatial-structure refs (storey, building) via its own expandSpatialRef, but the result never passed through cameraCallbacks.resolveHighlightIds. Isolating a geometry-less IfcElementAssembly by ref therefore isolated an id with no mesh — the same #2532 failure mode (blank viewport), just in a channel the issue's audit didn't enumerate.

Fix

isolate() now resolves its assembled global ids through state.cameraCallbacks.resolveHighlightIds — the same choke point the other four channels use — falling back to the unresolved ids when no renderer has registered one yet, matching every other channel's own fallback.

Root cause

cameraCallbacks.resolveHighlightIds is an optional callback each channel has to remember to call; a new channel that skips it compiles fine and silently isolates un-expanded ids.

This PR closes the one instance of that already found live in the tree. It does not add a structural (compile-time) guarantee against a future sixth channel forgetting — that's the open design question #3338 raises, and is out of scope here.

Test plan

  • RED: visibility-adapter.assembly.test.ts fails against the pre-fix isolate() (isolates the raw assembly id [42] instead of its parts [9001, 9002])
  • Proved not vacuous: a second test pins the pre-fix fallback behavior explicitly (no resolver registered → raw ids), so the first assertion's failure was contentful, not a bad fixture
  • GREEN after the fix; expandToGeometryBearingIds/aggregation.test.ts unaffected (53/53 pass)
  • pnpm typecheck (root, turbo) — 1503/1503 test files in program, 0 errors
  • pnpm lint — 0 errors
  • node scripts/check-module-size.mjs — OK
  • node scripts/check-source-text-assertions.mjs — OK, 0 new
  • Changeset added (@ifc-lite/viewer patch)

🤖 Generated with Claude Code

…ansion resolver (#3338)

expandToGeometryBearingIds has one production call site the codebase knows
about, cameraCallbacks.resolveHighlightIds, which LensPanel, PropertiesPanel
and both SearchModal isolate paths already route through. The SDK/MCP
visibility adapter's isolate() (scripts, the viewer_isolate tool) is a fifth
channel, not named in #3338: it expands spatial-structure refs but never
routed the result through the same resolver, so isolating a geometry-less
IfcElementAssembly by ref left an id with no mesh in the isolation set and
showed an empty viewport — the same #2532 failure mode, in an unenumerated
channel.

isolate() now resolves through cameraCallbacks.resolveHighlightIds the same
way the other four channels do, falling back to the unresolved ids when no
renderer has registered one yet (matching every other channel's fallback).
@BIMvoice
BIMvoice requested a review from louistrue as a code owner August 28, 2026 07:04
@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Warning

Review limit reached

  • Run on-demand review

This review includes 3 billable files and costs up to $0.75.

Or wait 14 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: 1b3b6280-9801-4a0b-bb9b-8dfd81f9cf28

📥 Commits

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

📒 Files selected for processing (3)
  • .changeset/isolate-assembly-expansion.md
  • apps/viewer/src/sdk/adapters/visibility-adapter.assembly.test.ts
  • apps/viewer/src/sdk/adapters/visibility-adapter.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 1750ms 2905ms -39.8% +50%
firstVisibleGeometryMs 2472ms 3652ms -32.3% +50%
streamCompleteMs 2867ms 3598ms -20.3% +50%
spatialReadyMs 1199ms 1032ms +16.2% +50%
metadataCompleteMs 1863ms 3063ms -39.2% +50%
totalWallClockMs 3000ms 3700ms -18.9% +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 320ms 1075ms -70.2% +50%
firstVisibleGeometryMs 1079ms 1572ms -31.4% +50%
streamCompleteMs 919ms 1980ms -53.6% +50%
spatialReadyMs 945ms 915ms +3.3% +50%
metadataCompleteMs 1016ms 1392ms -27.0% +50%
totalWallClockMs 1200ms 3300ms -63.6% +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.

1 participant