test(viewer): pin the add-element preview to the renderer's frame (#2802) - #3058
Conversation
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_0fdc429f-bb5f-4af9-975d-e175861b5a3e) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughAdded tests for ChangesMesh frame coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change adds targeted coverage for renderer-frame coordinate mapping without altering production behavior; no actionable merge-blocking risk remains after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
|
CodeRabbit CLI run locally against Recording the run shape rather than only the verdict, because a rate-limited CLI also exits 0 with no findings and the two are indistinguishable from the exit code alone. The sandbox phase, the 25 seconds of analysis and the named file are what make this a review rather than a skip. Worth noting in this specific case: eight other open PRs currently report every check green while having zero CodeRabbit inline comments, zero review records, and a rate-limit marker. So on this repo right now, "the bot check is green" and "the bot reviewed it" are not the same statement. Pre-flight for the exact published commit: |
a53c1e8 to
a3f88c4
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_4e64a41c-7f48-4e22-bbc8-b004307e85fc) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a53c1e847d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| near(n[24], 0, 'side normal x'); | ||
| near(n[25], 0, 'side normal y'); | ||
| near(n[26], -1, 'side normal z (IFC +Y becomes renderer -Z)'); |
There was a problem hiding this comment.
If buildBoxFromIfcCorners reorders its geometrically equivalent faces or vertices, the renderer output remains correct but this assertion fails because it assumes that the first side face always begins at normal offset 24. Assert that the mesh contains the expected mapped side normal, or associate normals with positions, so the test verifies frame conversion without making internal vertex ordering a compatibility contract.
AGENTS.md reference: AGENTS.md:L122-L123
Useful? React with 👍 / 👎.
| corners: [ | ||
| [2, 5, 1], | ||
| [8, 5, 3], | ||
| [2, 9, 7], |
There was a problem hiding this comment.
The two production paths cannot supply these differing corner Z values: mutationSlice.ts's profileCornersFromParams gives every corner the same Z, while useSpaceGhostPreview.ts explicitly maps every corner to Z=0. Consequently, this fixture pins the arbitrary implementation detail that non-planar input is flattened to corner[0]; a refactor that preserves every production preview but carries individual Z values or rejects malformed non-planar input would fail the test. Keep the asymmetric X/Y and nonzero storey elevation, but use a planar footprint so the test asserts the actual frame-mapping invariant.
AGENTS.md reference: AGENTS.md:L122-L123
Useful? React with 👍 / 👎.
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 |
a3f88c4 to
9d0eac3
Compare
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_bb925927-ab86-45ae-b73a-3750b9e40b44) |
`addElementMeshes.ts` is 365 lines with two production callers and no test of any kind. It is one of the slices #2802 lists as reached by no fixture, and the largest of them by a wide margin — the other five are setters. What it does is convert IFC storey-local coordinates into the renderer's frame: renderer.x = ifc.x renderer.y = ifc.z + storeyElevation renderer.z = -ifc.y Three of the ways that can break are INVISIBLE to a fixture built from convenient numbers. A preview at the origin, on a storey at elevation 0, with a square footprint, maps to itself under an axis swap, a dropped negation and a dropped elevation alike. That is the same blindness as #3013's uniform wall thickness and #2913's opposite-edge swap: the fixture shares a symmetry with the bug. So every number in the fixture is distinct and non-zero, and the test asserts that rather than hoping for it. ## The rule is transcribed TWICE, and my first version pinned one copy `buildPolygonExtrusion` does not share `buildBoxFromIfcCorners`. It carries its own copy of `(x, z + elevation, -y)`, serving slab, roof, plate and space. The first three tests here all routed through the box path, so dropping the elevation or the negation in the polygon copy failed nothing. That is not a symmetric gap. Of the two production callers, `useSpaceGhostPreview` builds ONLY `type: 'space'` payloads — so one entire caller exercised exclusively the copy the tests missed, and the file's own header claimed coverage it did not have. Two transcriptions of one rule with no gate is the shape this repo keeps getting caught by. Pinning both is the cheap half. Collapsing them into one is the real fix and belongs in a change that touches production code. ## And the frame mapping is not the only transcription in the file Review found three more, each a separate hand-written rule that the frame tests could not see, and each one already mutable without a red: - `buildLinearBox` computes a perpendicular offset and reads BOTH endpoints' Z so a sloped beam previews sloped. The source comment says so explicitly. Pinning the base to `startIfc[2]` broke nothing. - `space` and `slab` take DIFFERENT lines in the dispatch (`params.Height` versus `params.Thickness`). The polygon test cited `useSpaceGhostPreview`, which builds only `space`, and then used a `slab` fixture — so it argued for covering a caller it did not cover. It now uses `space`. - door and window call `buildAxisBox(..., Width, FrameThickness, Height)` where column passes `(Width, Depth, Height)`. Swapping the last two gives every door a 0.05 m tall, 2 m deep preview, and nothing noticed. Mutation table, each confirmed to compile — and three of these first failed to APPLY because of a quoting slip, which prints identically to surviving. They were re-run: box path: positions z drops the negation -> 1 fails box path: positions y drops storeyElevation -> 1 fails box path: the two mapped axes swapped -> 1 fails box path: normals z sign flipped -> 1 fails POLYGON path: drops storeyElevation -> 1 fails POLYGON path: drops the negation -> 1 fails POLYGON path: baseZ from the last corner -> 1 fails POLYGON path: per-corner z, not flattened -> 1 fails beam base pinned to the start endpoint -> 1 fails space reads the wrong param -> 1 fails zero-height guard deleted -> 1 fails door argument order swapped -> 1 fails The normals case earns its own assertion because a normal left in the IFC frame lights the preview from the wrong side, which reads as a material bug rather than a frame bug and gets investigated in the wrong place. The degenerate-wall case is the control: the other tests assert a mesh IS built, which would also hold if this function built one for anything at all. Review also caught that the `as never` casts on the params fixtures were not load-bearing — they silenced excess-property checking, which was the only type check these fixtures had. Removed, and verified by the negative control: with the cast gone, a typo'd field (`Thicknes`) now fails `tsc`, where before it compiled and would have produced NaN geometry silently. Removing them then exposed a real one I had not looked for: the slab fixture supplied only `Thickness` where the type needs `{ Width, Depth, Thickness }`. `tsx` strips types without checking, so the tests were green while the `Typecheck` gate would have failed — the green-tests, red-typecheck split. The lesson is narrow and mine: I removed the suppression and did not re-run the check it had been suppressing. Two assertions were also weaker than they read. The normals test asserted only the bottom face, whose IFC normal is `[0, 0, -1]`, so both mapped components are zero there and the `-face.normal[1]` term was untested; it now also pins a side face. And the polygon fixture put all three corners at one z, which cannot distinguish "base from corner[0]" from "base from any corner" or from per-corner z — the corners are now at distinct heights. Does not close #2802. It covers one of the six remaining slices; the other five are almost entirely plain setters, where a test would assert `set(x)` then `get() === x` and could not fail for a reason that matters. The three genuinely failable properties in that group are recorded on the issue rather than written as slice coverage. addElementMeshes.frame.test.ts 7 passed, 0 failed tsc --noEmit -p apps/viewer/tsconfig.json clean for this file
9d0eac3 to
865fd4c
Compare
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_1df8fbca-2b75-4783-8507-d128e472749a) |
1 participant
One of the six slices #2802 lists as reached by no fixture. Does not close it — the other five are covered by a note on the issue explaining why they are declined rather than pending.
Why this one and not the other five
addElementMeshes.tsis 365 lines with two production callers and no test of any kind, and it does real work rather than storing values. It converts IFC storey-local coordinates into the renderer's frame:The other five slices are almost entirely
setX: (x) => set({ x }). A test there assertsset(x)thenget() === x, which pins the language rather than the code.The blindness this is built against
An axis swap, a dropped negation and a dropped storey elevation are all invisible to a fixture at the origin, with a square footprint, on a storey at elevation 0 — that fixture maps to itself under every one of them. Same shape as #3013's uniform wall thickness and #2913's opposite-edge swap: the fixture shares a symmetry with the bug.
So every number is distinct and non-zero, and the column test asserts that rather than hoping for it.
What review found that I had not
The rule is transcribed more than once. My first version pinned
buildBoxFromIfcCornersand declared the job done.buildPolygonExtrusioncarries its own copy of the same mapping, and of the two production callersuseSpaceGhostPreviewbuilds onlyspacepayloads — so one entire caller was exercising exclusively the copy the tests missed, while the file's header claimed coverage.Three further separate transcriptions were also unpinned:
buildLinearBox's perpendicular offset and both-endpoint-Z handling (whose source comment states the behaviour with nothing testing it), thespaceversusslabdispatch reading different params, and the per-type argument order intobuildAxisBox— swapping two of those gives every door a 0.05 m tall, 2 m deep preview.A green-tests, red-typecheck split. Removing the unnecessary
as nevercasts was right — they were suppressing the only type check those fixtures had. But it exposed a real error I did not then look for: the slab fixture supplied onlyThicknesswhere the type needs{ Width, Depth, Thickness }.tsxstrips types without checking, so tests passed while theTypecheckgate would have failed. The lesson is narrow and mine: I removed a suppression and did not re-run the check it had been suppressing.Two assertions weaker than they read. The normals test asserted only the bottom face, whose IFC normal is
[0, 0, -1]— both mapped components are zero there, so the-face.normal[1]term was untested. And the polygon fixture put all three corners at one z, which cannot distinguish "base fromcorner[0]" from "base from any corner" or from per-corner z.Mutation table
Each confirmed to compile. Three first failed to apply because of a quoting slip, which prints identically to surviving — they were re-run.
The degenerate-wall case is the control on "a mesh IS built", and the beam test covers its mirror — a guard that rejects valid input leaves that control green too, and in this repo that is the more common of the two.
Verification
Test-only diff; no production code changed. Pre-flight:
/simplifythen/code-review, everything they raised applied and mutation-verified.Collapsing the two frame transcriptions into one is the real fix and belongs in a change that touches production code; noted rather than done here.
Summary by CodeRabbit