fix(viewer): guide users to load the model before a BCF's viewpoints resolve (#4099) - #4133
fix(viewer): guide users to load the model before a BCF's viewpoints resolve (#4099)#4133BIMvoice wants to merge 3 commits into
Conversation
…resolve (#4099) A BCF's topics reference GlobalIds in the model they were captured from, which readBCF never checks — importing succeeds regardless of what (if anything) is loaded, so a BCF's viewpoints silently fail to resolve later with nothing to explain why. BCFPanel now warns at import time when no model is loaded. Separately, dropping a .bcfzip file onto the main viewport matched neither a supported model extension nor the .zip branch of the unsupported-format explainer (".bcfzip" has no literal dot before "zip"), so the drop did nothing at all — exactly the "maybe it should be unzipped?" confusion quoted in the issue. describeUnsupportedFormat now names BCF archives and points at the BCF panel's Import button. describeUnsupportedFormat moved out of pointCloudIngest.ts (at its module-size budget) into a new sibling module, unsupportedFormat.ts, since it has no dependency on the point-cloud pipeline. docs/guide/bcf.md's Viewer Integration section claimed drag-and-drop BCF import works; it doesn't (no drop handler exists in BCFPanel or its subcomponents) — corrected to describe the Import button and the load order. Closes #4099 Discoverability (the issue's other half) is left to the maintainer: a free BCF viewer isn't obviously findable outside this repo, and that's a landing-page/marketing/positioning call, not one to make unilaterally from the viewer codebase. docs/guide/bcf.md already states BCF 2.1/3.0 support and topic/viewpoint rendering, and is linked from mkdocs.yml's nav, so the docs side is already discoverable once someone is in the docs; broader discoverability (e.g. landing-page copy) is a suggestion for the maintainer to weigh, not something this PR changes.
|
Warning Review limit reached
This review includes 9 billable files and costs up to $2.25. Or wait 59 minutes for your next included review. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
Comment |
Claude review - no findings for
|
Viewer benchmark✅ No threshold regressions detected. 01_Snowdon_Towers_Sample_Structural(1).ifcBaseline recorded 2026-07-01T20:31:05.538Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
AC20-FZK-Haus.ifcBaseline recorded 2026-07-01T20:30:59.972Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
Refresh the baseline from a CI run: dispatch the Benchmark workflow with |
|
#4133 partially addresses #4099. The issue has two asks:
Since only one of the two items is delivered, I've changed the PR description from @louistrue — requesting the |
|
Applying
That is the right call by the author, and I am not going to ask for it to be changed. Writing The work itself is queue-approved: the referenced issue carries Label applied by the maintainer account, not by the PR author, so it is not the self-applied shape the gate is written to reject. Filed the underlying tension separately: a gate that demands a closing reference pressures an author toward either over-claiming or getting blocked, and honest partial work is the population it punishes. That is worth fixing in the gate rather than fixing in every PR. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Claude review - no findings for
|
Claude review - no findings for
|
Refs #4099 — load-order guidance is done; the discoverability half (landing-page/README framing naming BCF viewing as a standalone use case) is intentionally left to the maintainer, not implemented here.
Context
The issue has two halves. This PR addresses the concrete, substantive one (load order) and does the defensible low-risk part of the discoverability half; it deliberately leaves the rest of discoverability to the maintainer.
(a) Load-order guidance — implemented
A BCF's topics reference GlobalIds in the model they were captured from.
readBCFnever checks this, so importing a BCF succeeds regardless of what — if anything — is loaded in the viewport. With no model loaded, every viewpoint then silently fails to resolve later (nothing to zoom to or select), with no error explaining why.BCFPanel.tsx'shandleImportFilenow callswarnIfNoModelLoaded(models.size)right after a successful import, which shows atoast.info("BCF imported. Load the model this BCF refers to, to view its topics' viewpoints in 3D.") when no model is loaded. No toast when a model is already loaded (normal path untouched)..bcfzipfile directly onto the main viewport used to do nothing at all:.bcfzipmatches neitherisSupportedModelFile(it's not a model) nor the.zipbranch of the unsupported-format explainer — the string"bcfzip"has no literal dot before"zip", soendsWith('.zip')isfalse. This is exactly the confusion quoted in the issue ("Maybe it should be unzipped? Isn't it .bcf format?").describeUnsupportedFormatnow recognizes.bcf/.bcfzipand names the fix: use the BCF panel's Import button, load the model first if you haven't.docs/guide/bcf.md's Viewer Integration section claimed "Drag and drop a.bcfor.bcfzipfile" works. It doesn't — there is no drop handler inBCFPanel.tsxor its subcomponents (verified: noonDrop/DragEvent/dataTransferanywhere in that tree). Corrected to describe the actual Import-button flow and the load-order requirement.Refactor note
describeUnsupportedFormatmoved out ofpointCloudIngest.tsinto a new sibling moduleunsupportedFormat.ts—pointCloudIngest.tswas sitting at its module-size budget (line-count, zero headroom) and this function has no dependency on the point-cloud pipeline it lived next to.ViewportContainer.tsx's import updated accordingly.Tests + mutation
apps/viewer/src/hooks/ingest/unsupportedFormat.bcf.test.ts— asserts.bcf/.bcfzipget a BCF-specific message, and don't fall through to the generic "extract first" ZIP message.apps/viewer/src/components/viewer/BCFPanel.import-load-order.test.tsx— drives the realBCFPanelcomponent (realreadBCF/writeBCFround trip, not mocked) via its actual file-input change event, against an empty store: asserts exactly onetoast.infoguidance call, and separately asserts no toast when a model is already loaded (regression check on the normal path)..bcf/.bcfzipbranch) and confirmed all 5 new/touched assertions go RED, then restored and confirmed GREEN.(b) Discoverability — not unilaterally redesigned
The issue's core observation — people ask for a free BCF viewer without knowing ifc-lite is one — is a landing-page/marketing/positioning question. That's the maintainer's call, and I have not touched any landing-page copy or invented product claims.
What I did verify and confirm is already true:
docs/guide/bcf.mdalready states plainly, in its first paragraph, that@ifc-lite/bcfimplements BCF 2.1 and 3.0 — confirmed againstpackages/bcf/src/writer.ts, which branches onversion: '2.1' | '3.0'throughout (writeVersionFile,writeProjectFile, markup-writing) with version-specific schema handling for both.packages/bcf'scomputeMarkerPositions+ DOM renderer, wired intoBCFOverlay).docs/guide/bcf.mdis already linked frommkdocs.yml's nav (User Guide > BCF Collaboration), andREADME.mdalready links to it and lists "BCF topic tracking" as a feature.So the docs-side ask in the issue ("state clearly that the viewer opens BCF 2.1/3.0 and renders topics with viewpoints, and make sure the doc is discoverable") was already true before this PR, apart from the drag-and-drop overclaim fixed in (a) above.
Suggestion for the maintainer, not implemented here: if there's appetite to be the visible answer to "free BCF viewer" queries, that likely means landing-page copy or README framing naming BCF viewing as a standalone use case — a product/positioning decision I've left alone.
#4097 conflict check
Checked PR #4097 (BCF panel "issues" → "topics" terminology rename) before starting. It is already merged into
upstream/main(commitf48b803ee), which this branch is based on — so there's no live conflict to manage. None of my changes touch renamed strings; the panel heading, registry title, and BCFPanel's topic vocabulary are all untouched by this PR.Gates
node scripts/check-module-size.mjs— OK, 0 new over budget (both edited files brought back to/under their line budgets via the extraction above)node scripts/check-test-wiring.mjs— OKnode scripts/check-source-text-assertions.mjs— OK, 0 newturbo run typecheck --filter=@ifc-lite/viewer— 41/41 tasks successfulturbo run test --filter=@ifc-lite/viewer— 7146 tests, 7140 pass, 0 fail, 6 skipped (5 of the 7146 are the new tests added by this PR; no regressions)What I ran vs. skipped
Ran the full viewer suite via turbo (not
pnpm --filter) per AGENTS.md. Skipped running the full monorepo test matrix (other packages untouched by this diff) — only@ifc-lite/viewerfiles changed plus docs.git status --porcelainis clean.