fix(viewer): honour Show Annotations toggle for IfcAnnotation 3D meshes#1356
Conversation
… meshes IfcAnnotation entities can carry real 3D solid geometry (e.g. Bonsai plan-view "DRAWING" boxes) alongside the 2D symbolic curve overlay. The ifcAnnotations toggle only drove the curve overlay, so those 3D meshes rendered as stray grey cubes that ignored the Show Annotations setting. Gate IfcAnnotation meshes by typeVisibility.ifcAnnotations at the three existing type-visibility filter sites (merged-geometry render filter, visible-basket set, and GLB visible-only export) so the toggle hides both representations consistently. Fixes #1354
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds ChangesIfcAnnotation Visibility Support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/viewer/src/store/basketVisibleSet.test.ts (1)
264-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
models-backed case for this toggle.These assertions only cover the legacy
state.geometryResultfallback. The same visibility gate also runs through thestate.modelspath, so this change should be locked down with at least onemodels.size === 1/federated case as well. As per coding guidelines, "Resolve selections/IDs throughFederationRegistry(toGlobalId/fromGlobalId/getModelForGlobalId), never ad-hoc math; honor the single-model fallbackglobalId === expressId. Verify behaviour atmodels.sizeof 1 and N."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/viewer/src/store/basketVisibleSet.test.ts` around lines 264 - 293, The IfcAnnotation visibility toggle is only covered through the legacy geometryResult fallback, so add a models-backed test case to lock down the same behavior in the state.models path. Update the basket visibility tests around getVisibleBasketEntityRefsFromStore and invalidateVisibleBasketCache to include at least one federated setup with models.size === 1 (and, if practical, an N-model case) so the toggle is verified when IDs are resolved through FederationRegistry. Keep the assertions aligned with the existing ifcAnnotations on/off behavior and ensure the models path uses the same selection/ID resolution flow as the rest of the store.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/viewer/src/store/basketVisibleSet.test.ts`:
- Around line 270-285: The tests are bypassing the GeometryResult type by
casting geometryResult as any in basketVisibleSet.test.ts, which makes the
fixture brittle. Replace those ad hoc casts with a small typed helper or shared
fixture that returns a correctly shaped geometryResult for the relevant test
cases, and use it wherever setState is populating geometryResult in this spec.
---
Nitpick comments:
In `@apps/viewer/src/store/basketVisibleSet.test.ts`:
- Around line 264-293: The IfcAnnotation visibility toggle is only covered
through the legacy geometryResult fallback, so add a models-backed test case to
lock down the same behavior in the state.models path. Update the basket
visibility tests around getVisibleBasketEntityRefsFromStore and
invalidateVisibleBasketCache to include at least one federated setup with
models.size === 1 (and, if practical, an N-model case) so the toggle is verified
when IDs are resolved through FederationRegistry. Keep the assertions aligned
with the existing ifcAnnotations on/off behavior and ensure the models path uses
the same selection/ID resolution flow as the rest of the store.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 851889a3-55fd-4294-913f-88f5ca397025
📒 Files selected for processing (4)
apps/viewer/src/components/viewer/GLBExportDialog.tsxapps/viewer/src/components/viewer/ViewportContainer.tsxapps/viewer/src/store/basketVisibleSet.test.tsapps/viewer/src/store/basketVisibleSet.ts
| geometryResult: { meshes } as any, | ||
| typeVisibility: { ...useViewerStore.getState().typeVisibility, ifcAnnotations: true }, | ||
| }); | ||
| invalidateVisibleBasketCache(); | ||
|
|
||
| const refs = getVisibleBasketEntityRefsFromStore(); | ||
| assert.ok(refs.some((r) => entityRefToString(r) === 'legacy:2')); | ||
| }); | ||
|
|
||
| it('drops IfcAnnotation 3D meshes when the toggle is off', () => { | ||
| useViewerStore.setState({ | ||
| selectedEntitiesSet: new Set(), | ||
| selectedEntity: null, | ||
| selectedEntityIds: new Set(), | ||
| hierarchyBasketSelection: new Set(), | ||
| geometryResult: { meshes } as any, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== GeometryResult declarations =="
rg -n -C2 "export (type|interface) GeometryResult|type GeometryResult|interface GeometryResult" apps/viewer/src -g '**/*.ts' -g '**/*.tsx'
echo
echo "== New casts in basketVisibleSet.test.ts =="
rg -n -C2 "geometryResult: \{ meshes \} as any|as any" apps/viewer/src/store/basketVisibleSet.test.ts
echo
echo "== Existing typed geometry fixtures in tests =="
rg -n -C2 "GeometryResult|geometryResult:" apps/viewer/src -g '**/*.test.ts' -g '**/*.test.tsx'Repository: LTplus-AG/ifc-lite
Length of output: 15800
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== basketVisibleSet.test.ts around the casts =="
sed -n '1,360p' apps/viewer/src/store/basketVisibleSet.test.ts
echo
echo "== GeometryResult / MeshData declarations =="
fd -a 'index.ts' apps -t f | rg '`@ifc-lite/geometry`|geometry' || true
rg -n -C3 "export (type|interface) GeometryResult|export (type|interface) MeshData|type GeometryResult|interface GeometryResult|type MeshData|interface MeshData" . -g '**/*.ts' -g '**/*.tsx'
echo
echo "== Where geometryResult is typed in viewer store types =="
rg -n -C3 "geometryResult" apps/viewer/src/store -g '**/*.ts' -g '**/*.tsx'Repository: LTplus-AG/ifc-lite
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== basketVisibleSet.test.ts around the casts =="
sed -n '1,360p' apps/viewer/src/store/basketVisibleSet.test.ts
echo
echo "== GeometryResult / MeshData declarations =="
rg -n -C3 "export (type|interface) GeometryResult|export (type|interface) MeshData|type GeometryResult|interface GeometryResult|type MeshData|interface MeshData" . -g '**/*.ts' -g '**/*.tsx'
echo
echo "== Where geometryResult is typed in viewer store types =="
rg -n -C3 "geometryResult" apps/viewer/src/store -g '**/*.ts' -g '**/*.tsx'Repository: LTplus-AG/ifc-lite
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== basketVisibleSet.test.ts around the casts =="
sed -n '1,360p' apps/viewer/src/store/basketVisibleSet.test.ts
echo
echo "== GeometryResult / MeshData declarations =="
rg -n -C3 "export (type|interface) GeometryResult|export (type|interface) MeshData|type GeometryResult|interface GeometryResult|type MeshData|interface MeshData" . -g '**/*.ts' -g '**/*.tsx'
echo
echo "== Where geometryResult is typed in viewer store types =="
rg -n -C3 "geometryResult" apps/viewer/src/store -g '**/*.ts' -g '**/*.tsx'Repository: LTplus-AG/ifc-lite
Length of output: 50374
Replace the geometryResult casts with a typed fixture/helper
apps/viewer/src/store/basketVisibleSet.test.ts uses as any for geometryResult in several places. That bypasses the GeometryResult shape and makes the test brittle if the fixture drifts. Use a small typed helper/fixture instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/viewer/src/store/basketVisibleSet.test.ts` around lines 270 - 285, The
tests are bypassing the GeometryResult type by casting geometryResult as any in
basketVisibleSet.test.ts, which makes the fixture brittle. Replace those ad hoc
casts with a small typed helper or shared fixture that returns a correctly
shaped geometryResult for the relevant test cases, and use it wherever setState
is populating geometryResult in this spec.
Source: Coding guidelines
…y gate Addresses CodeRabbit review on #1356: the prior cases only exercised the legacy geometryResult fallback; add a state.models-backed case so the collectVisibleCandidates models path is locked down too.
1 participant
Problem
Closes #1354.
IfcAnnotationentities can carry real 3D solid geometry — e.g. the Bonsai plan-view "DRAWING" boxes in the issue render as grey cubes. TheShow Annotationstoggle (typeVisibility.ifcAnnotations) only drove the separate 2D symbolic curve overlay (useSymbolicAnnotationsinViewport.tsx). The 3D meshes flow through the normal merged-geometry path, whose type filter handledIfcSpace/IfcOpeningElement/IfcVirtualElement/IfcSitebut neverIfcAnnotation— so the cubes always rendered and ignored the toggle.Fix
Gate
IfcAnnotationmeshes ontypeVisibility.ifcAnnotationsat the three existing type-visibility filter sites (the same three the other five toggles already live in):ViewportContainer.tsx— merged-geometry render filter (+ change-detection andneedsFilterguards)basketVisibleSet.ts—matchesTypeVisibility(selection / list / isolation visible set)GLBExportDialog.tsx—buildHiddenIfcTypes(visible-only export parity)Turning Annotations off now hides both the 2D overlay and the 3D cubes consistently. Default stays
true, so users who want annotations see no change.This does not tie anything to the section plane — it's a pure global type filter, consistent with the "annotations toggle is global" guidance.
Scope note
This makes the toggle effective; the cubes still show by default. Flipping the default so Bonsai drawing annotations are hidden out-of-the-box is a separate decision (it would also hide legitimate annotations), left out of this PR.
Tests
basketVisibleSettests for Plane/cross section drawings from Bonsai showing as cubes #1354 (include-when-on / drop-when-off). Negative control confirmed the drop test fails without the gate.basketVisibleSet(16) andvisibilitySlice(29) suites green.Summary by CodeRabbit