fix(viewer): main is red — the frame test pinned the pre-#3039 side normal

[louistrue]

main is red and has been since deaf4f088. Twelve completed Test runs have failed, all on the same single assertion:

apps/viewer/src/store/slices/addElementMeshes.frame.test.ts
  not ok 2 - maps normals through the same frame change as positions
    error: 'side normal z (IFC +Y becomes renderer -Z): expected -1, got 1'
  # tests 1772   # pass 1771   # fail 1

A pair: each green alone, red together

• test(viewer): pin the add-element preview to the renderer's frame (#2802) #3058 added this test, pinning a side-face normal.
• fix(viewer): outward-facing normals on the add-element preview box sides #3039 then made side normals point outward, orienting each face away from the box centre.

Neither is wrong by itself. #3058 was correct about the code as it then stood. Only the merged result is wrong, which is why per-PR verification could not catch it.

#3039 is right; the test pinned the inward normal

Computed against the real corner order rather than re-derived by eye:

column Width=1 Depth=2 Height=3, buildAxisBox
box centre       (0,  0, 1.5)
face [0,4,5,1]   all four corners at y = -1, centre (0, -1, 1.5)
outward normal   (0, -1, 0) IFC  ->  renderer (0, 0, +1)
test expected    (0, +1, 0) IFC  ->  renderer (0, 0, -1)

That face is the −Y side of the box, so outward points at −Y. The old expectation pinned inward, and it passed because the side normals were inward — the defect #3039 fixed.

So this is the one case where editing an assertion is correct: the behaviour genuinely changed and the assertion was pinned to the old, wrong behaviour.

Reverting #3039 is not available, and outward is substantively right

addElementMeshes.test.ts:148-262, also from #3039, is a general property test that every vertex normal of every box shape points away from the centre. Against inward normals it fails 6 assertions. Reverting would mean deleting that file too.

It also matters visually. The renderer draws with cullMode: 'none', so backfaces are unaffected — but the shader takes its shading normal's sign from the vertex normal (main.wgsl.ts:433-435, N = N * sign(alignDot)). The diffuse terms use abs() and do not care, but the rim light max(dot(N, rimLight), 0.0) and the transparency fresnel do. With rimLight = normalize(0, 0.2, -1), the −Y face's outward (0,0,1) gives dot ≈ -0.98 (no rim); inward gives +0.98 (full rim). The preview was rim-lighting exactly the faces that should not have it.

Proof it does not weaken the test

Changing an assertion demands showing it still discriminates, so the mutation was run rather than argued:

invert #3039's orientation (normals inward again)   1 of 7 fails
restored                                             7 of 7 pass
addElementMeshes.test.ts (#3039's own suite)       20 of 20 pass
whole apps/viewer/src/store/slices suite          823 of 823 pass

Nothing else pins the old direction. A repo-wide sweep of normal consumers — GPU packing, collab wire encode, GLB export, the three.js playground — found none expecting inward, and there are no snapshots or goldens encoding normals.

The comment now derives the geometry instead of asserting it

The old text stated IFC normal [0, 1, 0] as fact, which is what let a wrong sign survive review. It now shows the corner indices, the two centres and the difference, so a reader can check it in place.

Why this was invisible

Both PRs were verified green before merging and both verifications were correct. Only a completed run on the merged result can see a pair like this, and main's Test queue was 18 commits deep when both landed. Merge cadence below one commit per test-duration does not degrade that signal, it removes it.

No changeset: test-only, as #3058 was.

Summary by CodeRabbit

• Bug Fixes
  • Corrected side-face orientation so outward-facing normals are rendered properly.
  • Fixed IFC -Y normals mapping to the renderer’s expected +Z direction.

[cursor]

Bugbot couldn't run - usage limit reached

Bugbot 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_8ea4a6f0-47ca-498a-9b91-0fbba6b6e002)

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: aadd2788-5acc-4404-897f-01abb8438ebd

📥 Commits

Reviewing files that changed from the base of the PR and between b60e95c and f2c2bc9.

📒 Files selected for processing (1)

• apps/viewer/src/store/slices/addElementMeshes.frame.test.ts

Included review availability: 1 review is currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour.

---

📝 Walkthrough

Walkthrough

The regression test now verifies that the outward IFC -Y side-face normal maps to renderer-space +Z after mesh face orientation correction.

Changes

Mesh normal correction

Layer / File(s)	Summary
Outward side-face normal test apps/viewer/src/store/slices/addElementMeshes.frame.test.ts	The test expects the renderer-space Z component to be +1 and documents the corrected face orientation.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to f2c2b

This is a localized test-only correction for the updated outward-normal behavior; no actionable merge-blocking risk remains after normal checks and review.

Poem

A rabbit checks the mesh at night,
“The side face points outward right!”
Minus Y turns plus Z,
The test now agrees with ease,
And hops away beneath the light.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the failing viewer frame test and its outdated side-normal expectation.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

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	1342ms	2905ms	-53.8%	+50%	✅
firstVisibleGeometryMs	1887ms	3652ms	-48.3%	+50%	✅
streamCompleteMs	2088ms	3598ms	-42.0%	+50%	✅
spatialReadyMs	1172ms	1032ms	+13.6%	+50%	✅
metadataCompleteMs	1388ms	3063ms	-54.7%	+50%	✅
totalWallClockMs	2500ms	3700ms	-32.4%	+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	268ms	1075ms	-75.1%	+50%	✅
firstVisibleGeometryMs	789ms	1572ms	-49.8%	+50%	✅
streamCompleteMs	773ms	1980ms	-61.0%	+50%	✅
spatialReadyMs	825ms	915ms	-9.8%	+50%	✅
metadataCompleteMs	889ms	1392ms	-36.1%	+50%	✅
totalWallClockMs	800ms	3300ms	-75.8%	+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).

[louistrue]

Recording the merge decision before making it, so it is reviewable after the fact rather than a quiet merge.

Why this is being merged rather than held. Louis's standing instruction this session is to merge what is truly green, and fifteen-plus PRs have landed tonight on exactly that condition. This is one file, one assertion, one comment. Carving out an exception for the PR that repairs a break would be optics, not caution.

Why it is not the thing the rules forbid. "Do not tweak a test's expected value to get a pass" exists precisely because this change looks like that. The written exception is that an assertion may change when the behaviour genuinely changed and the assertion was pinned to the old behaviour. Claiming the exception is what everyone does, so the case is made from evidence:

• addElementMeshes.test.ts:148-262, from fix(viewer): outward-facing normals on the add-element preview box sides #3039, independently pins outward as a general property across every box shape. Reverting would require deleting that file too. Outward is a pinned convention, not a preference.
• The shader takes its shading normal's sign from the vertex normal (main.wgsl.ts:433-435). With rimLight = normalize(0, 0.2, -1), the −Y face's outward (0,0,1) gives dot ≈ -0.98 (no rim); inward gives +0.98 (full rim). The preview was rim-lighting exactly the faces that should not have it. That makes this a visible rendering defect, not a convention argument.
• Two genuinely independent verification routes: the geometry derived from origin/main without sight of this branch, and separately executed and mutation-tested here.
• A repo-wide sweep of normal consumers — GPU packing, collab wire encode, GLB export, three.js playground — found none expecting inward, and no snapshot or golden encodes normals.

Verified genuinely green, not fail=0 green:

runs on head   3
shards         4 registered, 4 passed
fail 0   pending 0

Shard 1 is the job that was failing on main. It ran and passed. shards: 0 with fail: 0 would have been the absent-CI false pass, on the PR fixing a break that only surfaced because a lane finally ran.

On who merges it. #3058 is a peer session's PR, and that session offered to merge this. It declared the conflict openly, which is the right thing to have done. Merging it here instead, because the conflict is real and removing it costs nothing: I wrote the fix and ran the verification, and I have no interest in the outcome beyond main being green.

The deeper point stays on the record: both PRs were verified green before merging and both verifications were correct. A pair like this is invisible from either side, and only a completed run on the merged result can see it. main's Test queue was 18 commits deep when both landed. Merge cadence below one commit per test-duration does not degrade that signal, it removes it.

[louistrue]

Merging this, and recording why here first so the decision is reviewable rather than a quiet merge.

Declaring a conflict of interest up front. The test this fixes is mine, from #3058. I wrote the wrong sign. My interest in that being repaired quickly points the same way as "main is red, land it", and when those align it should be said out loud rather than left for someone to notice.

Why it lands rather than waiting. The standing instruction this session is to merge what is truly green, and I have applied it to fifteen PRs tonight on that condition. This one meets it more rigorously than any of them. Carving out an exception specifically for the PR that repairs a break we caused would be optics, not caution — and every commit landing on main from here inherits a red lane until it goes in.

Verified three independent ways, which is the part that carries the decision.

1. Arithmetic, derived from origin/main before I saw this branch. buildAxisBox puts corners 0, 1, 4, 5 at cy - hy, so face [0,4,5,1] is the -Y face; outward IFC normal is [0,-1,0]; normals[z] = -face.normal[1] = +1. The old assertion of -1 pinned [0,+1,0], pointing into the box.
2. Behaviour, by the author: mutation run showing orientation inverted fails 1 of 7 and restored passes 7 of 7, so the corrected expectation still discriminates and is not a weakening.
3. CI, confirmed positively from the shard-1 log rather than from a green tick:

red main:   # tests 1772   # pass 1771   # fail 1
this PR:    # tests 1772   # pass 1772   # fail 0
            ok 2 - maps normals through the same frame change as positions

The identical test count on both sides is the bit that matters: the fix made the test pass, it did not make it disappear.

Why #3039 is right and this is not a test bent to fit code. "Do not change an assertion to get a pass" is exactly the rule this change resembles, so it has to clear the written exception — behaviour genuinely changed and the assertion was pinned to the old behaviour. It does:

• addElementMeshes.test.ts:148-262, also from fix(viewer): outward-facing normals on the add-element preview box sides #3039, independently pins outward as a general property over every box shape. Outward is a pinned convention, not a preference.
• The shading normal takes its sign from the vertex normal. With rimLight = normalize(0, 0.2, -1), the -Y face outward gives dot ~= -0.98 (no rim) and inward gives +0.98 (full rim), so the preview was rim-lighting exactly the faces that should not have it. A visible defect, not a convention argument.
• A repo-wide consumer sweep found nothing expecting inward, and no snapshot or golden encodes normals at all.

CodeRabbit here is a real review, not a bare tick. 0 inline comments would normally read as a false pass, and I checked the body rather than the count: it names the commit range b60e95c2 -> f2c2bc93, lists the file processed, carries an accurate walkthrough, and reports "1 review is currently available" rather than a limit marker. A genuinely clean review legitimately has zero inline comments, which is a case a count-based check gets wrong.

The comment change is the more important half of this PR. My original stated a conclusion — "IFC normal [0, 1, 0], which maps to renderer [0, 0, -1]" — that no reader could check without recomputing it, which is how a wrong sign survived my own review. The replacement derives it in place from the box centre and face centre. The defect was never the sign; it was an unverifiable assertion in a comment.

What this cost, on the record: 13 consecutive red heads on main, from deaf4f08 to b60e95c2. #3058 and #3039 were each green alone and red only together, so no per-PR check could have caught it. Only a completed run on the merged result could, and that signal was 18 commits behind because we were merging faster than the Test lane completes.