fix(viewer): surface the DXF and wall-split skip notices instead of dropping them - #3023
Merged
Conversation
Two dropped-diagnostic fixes: DXF import already counts unhandled entity types into underlay.skipped, but the panel only ever rendered warnings — "most of your drawing imported, N entities of type X did not" was silent except for a console.warn in the all-zero case. Wall split already counts openings whose placement couldn't be reassigned (they stay attached to the tombstoned source wall, so they can end up orphaned), but the split toast only ever read toLeft/toRight, dropping the skipped count entirely. Both now surface via the panel/toast that already exists for the sibling diagnostic, rather than new UI.
|
Warning Review limit reached
Next review available in: 55 minutes Limit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
Contributor
Viewer benchmark⚠ 1 metric(s) exceeded the regression threshold (advisory only, not blocking). 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 |
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
BIMvoice
added a commit
that referenced
this pull request
Aug 22, 2026
… too #3023 surfaced `openings.skipped` on the wall-split CLICK path (`handleSelectionClick`) and pinned it with a test. A wall split commits from two places: the Split tool's numeric-distance panel (`SplitNumericInput.tsx` — type a distance, press Enter or hit Cut) is the other one, and it kept its own inline copy of the "(N openings reassigned)" string, read only `toLeft`/`toRight`, and never touched `skipped`. So the diagnostic #3023 was named for was still dropped silently there, for the same openings, on the same `splitWallAtDistance` result. Both notices now come from a single EMITTER — `notifyWallSplit` in the new `wallSplitNotice.ts` — which both call sites invoke instead of composing their own toasts. An emitter rather than a pair of shared formatters is the point: a formatter is still something a call site can neglect to call, which is exactly how these two paths came apart in the first place. `wallSplitNotice.ts` imports nothing but `@/components/ui/toast`, so the panel does not take on `selectionHandlers.ts`'s store, globalId, polygon-clip and measureHandlers imports just to announce a split, and `selectionHandlers.ts` loses 18 net lines. RED verified by restoring only the inlined success toast at the numeric path and keeping the new test: not ok 1 - warns about openings the split could not reassign error: 'expected a skipped-openings notice, got: []' 1 fail / 1 pass; restored -> 2/2. Both tests assert the FULL toast string (`Wall split (1 opening reassigned) — Ctrl+Z to undo`) rather than a substring, in both directions — the warning when `skipped > 0`, silence when it is 0 — so neither path's wording can drift from the other's. The numeric-path test mounts through `@/test/render` (#2434) instead of re-growing its own `createRoot`/`act`/`mounted[]` boilerplate, which is the same anti-duplication argument this fix makes. Also corrected: an earlier revision of this branch rewrote the click-path comment to say `skipped` is "Populated when a placement chain fails to resolve (mutationSlice.ts)". Both halves were wrong. `store/slices/mutationSlice.ts` only forwards the count (line 1981); it is incremented at twelve distinct sites in `apps/viewer/src/lib/wall-opening-reassign.ts` (lines 123-203), of which just one is an unresolvable placement chain — the rest are an attribute the reader could not read or a reference the opening does not carry, and one is not a fault at all (an opening whose `PlacementRelTo` points elsewhere is skipped on purpose, because rewriting its parent placement would teleport it). The accurate version lives in `wallSplitNotice.ts`'s module doc rather than at a call site. Verification: the four viewer test files covering `selectionHandlers` and the split panel run 27 passed / 0 failed both before and after; `tsc --noEmit` clean for `apps/viewer`; oxlint clean on every changed file; `check-changesets`, `check-test-wiring`, `check-test-glob-coverage`, `check-source-text-assertions` and `check-unused-locals` all exit 0.
louistrue
pushed a commit
that referenced
this pull request
Aug 22, 2026
…split path too (#3074) * fix(viewer): report skipped openings on the typed-distance wall split too #3023 surfaced `openings.skipped` on the wall-split CLICK path (`handleSelectionClick`) and pinned it with a test. A wall split commits from two places: `SplitNumericInput.tsx` — type a distance, press Enter — is the other one, and it kept its own inline copy of the "(N openings reassigned)" string, read only `toLeft`/`toRight`, and never touched `skipped`. So the diagnostic #3023 was named for is still dropped silently there, for the same openings, on the same `splitWallAtDistance` result. Rather than copy the second string to the second site, both notices now come from shared formatters in selectionHandlers.ts — `formatOpeningReassignSuffix` (already extracted by #3023) and a new `formatSkippedOpeningsNotice` — so the two paths cannot report differently on the same summary again. RED verified by removing only the two new lines in `SplitNumericInput.tsx` and keeping the test: not ok 1 - surfaces the skipped-openings notice, matching the click path word for word 1 fail / 1 pass; restored → 2/2, and the existing `selectionHandlers.wallSplitToast.test.ts` stays 4/4. `tsc --noEmit` clean; `check-test-wiring` and `check-source-text-assertions` green. Also corrected while there: the click-path comment said `skipped` is "Computed by `reassignWallOpenings` on every split". It is populated only when a placement chain fails to resolve, leaving the zero default otherwise — which is why the toast is silent in the common case. * fix(viewer): report skipped openings on the typed-distance wall split too #3023 surfaced `openings.skipped` on the wall-split CLICK path (`handleSelectionClick`) and pinned it with a test. A wall split commits from two places: the Split tool's numeric-distance panel (`SplitNumericInput.tsx` — type a distance, press Enter or hit Cut) is the other one, and it kept its own inline copy of the "(N openings reassigned)" string, read only `toLeft`/`toRight`, and never touched `skipped`. So the diagnostic #3023 was named for was still dropped silently there, for the same openings, on the same `splitWallAtDistance` result. Both notices now come from a single EMITTER — `notifyWallSplit` in the new `wallSplitNotice.ts` — which both call sites invoke instead of composing their own toasts. An emitter rather than a pair of shared formatters is the point: a formatter is still something a call site can neglect to call, which is exactly how these two paths came apart in the first place. `wallSplitNotice.ts` imports nothing but `@/components/ui/toast`, so the panel does not take on `selectionHandlers.ts`'s store, globalId, polygon-clip and measureHandlers imports just to announce a split, and `selectionHandlers.ts` loses 18 net lines. RED verified by restoring only the inlined success toast at the numeric path and keeping the new test: not ok 1 - warns about openings the split could not reassign error: 'expected a skipped-openings notice, got: []' 1 fail / 1 pass; restored -> 2/2. Both tests assert the FULL toast string (`Wall split (1 opening reassigned) — Ctrl+Z to undo`) rather than a substring, in both directions — the warning when `skipped > 0`, silence when it is 0 — so neither path's wording can drift from the other's. The numeric-path test mounts through `@/test/render` (#2434) instead of re-growing its own `createRoot`/`act`/`mounted[]` boilerplate, which is the same anti-duplication argument this fix makes. Also corrected: an earlier revision of this branch rewrote the click-path comment to say `skipped` is "Populated when a placement chain fails to resolve (mutationSlice.ts)". Both halves were wrong. `store/slices/mutationSlice.ts` only forwards the count (line 1981); it is incremented at twelve distinct sites in `apps/viewer/src/lib/wall-opening-reassign.ts` (lines 123-203), of which just one is an unresolvable placement chain — the rest are an attribute the reader could not read or a reference the opening does not carry, and one is not a fault at all (an opening whose `PlacementRelTo` points elsewhere is skipped on purpose, because rewriting its parent placement would teleport it). The accurate version lives in `wallSplitNotice.ts`'s module doc rather than at a call site. Verification: the four viewer test files covering `selectionHandlers` and the split panel run 27 passed / 0 failed both before and after; `tsc --noEmit` clean for `apps/viewer`; oxlint clean on every changed file; `check-changesets`, `check-test-wiring`, `check-test-glob-coverage`, `check-source-text-assertions` and `check-unused-locals` all exit 0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two user-visible notices were computed and then dropped: DXF import skipped entity types silently, and a wall-split that skipped openings said nothing. Found on a never-raised branch; merges clean.
RED — surgical, reverting only the two behaviour hunks in
DxfUnderlayPanel.tsxand thetoast.infoblock inselectionHandlers.ts, keeping the newformatOpeningReassignSuffixexport so imports still resolve (a blunt revert would have failed at import, which is not a RED):2 fail / 11 pass. Restored → 13/13.
Full
apps/viewersuite 5609 tests, 0 fail, 6 skipped.tsc --noEmitclean.check-unused-locals,check-test-wiring,check-source-text-assertions,check-changesetsall green.Prose checked against the code rather than taken as read:
ingestDxfFilereally does logskippedonly in the zero-drawable-entity branch (hooks/ingest/dxfIngest.ts:94); theOpeningReassignSummary.skippeddocs match (lib/wall-opening-reassign.ts:48-50); andunderlay.skippedis non-optional and never rehydrated from storage, so theObject.keyscall cannot throw on a restored session.One comment that is strictly inaccurate, harmless but worth fixing: it says
skippedis "Computed byreassignWallOpeningson every split". It is actually populated when either half's placement chain fails to resolve (mutationSlice.ts:1966-1982), leaving the zero default otherwise — so the toast stays silent in the common case, which is the intended behaviour but not what the comment describes.🤖 Generated with Claude Code