From c9c5466b972090cebc280ee0ee578e0995f2adaf Mon Sep 17 00:00:00 2001 From: Petru Conduraru Date: Sat, 22 Aug 2026 11:53:01 +0300 Subject: [PATCH 1/2] fix(viewer): report skipped openings on the typed-distance wall split too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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. --- .../components/viewer/selectionHandlers.ts | 34 ++++-- .../SplitNumericInput.skipNotice.test.tsx | 107 ++++++++++++++++++ .../viewer/tools/SplitNumericInput.tsx | 18 ++- 3 files changed, 145 insertions(+), 14 deletions(-) create mode 100644 apps/viewer/src/components/viewer/tools/SplitNumericInput.skipNotice.test.tsx diff --git a/apps/viewer/src/components/viewer/selectionHandlers.ts b/apps/viewer/src/components/viewer/selectionHandlers.ts index 3a78b6ffc8..088407fcba 100644 --- a/apps/viewer/src/components/viewer/selectionHandlers.ts +++ b/apps/viewer/src/components/viewer/selectionHandlers.ts @@ -165,14 +165,12 @@ export async function handleSelectionClick(ctx: MouseHandlerContext, e: MouseEve // `openings.skipped` (wall-opening-reassign.ts) counts doors/windows // whose placement we couldn't interpret — they stay attached to the // now-tombstoned source wall rather than either half, so they can - // end up orphaned. Computed by `reassignWallOpenings` on every - // split; previously only `toLeft`/`toRight` ever reached this - // toast, so a skip was silent. - if (wallTry.openings.skipped > 0) { - toast.info( - `${wallTry.openings.skipped} opening${wallTry.openings.skipped === 1 ? '' : 's'} could not be reassigned and may need manual repositioning`, - ); - } + // end up orphaned. Populated when a placement chain fails to resolve + // (mutationSlice.ts), so the zero default — and this silence — is the + // common case; before #3023 only `toLeft`/`toRight` ever reached this + // toast, so a skip was silent even when it happened. + const skippedNotice = formatSkippedOpeningsNotice(wallTry.openings); + if (skippedNotice) toast.info(skippedNotice); return; } const linearTry = state.splitLinearElementAtDistance( @@ -969,6 +967,26 @@ export function formatOpeningReassignSuffix(op: { toLeft: number; toRight: numbe return moved > 0 ? ` (${moved} opening${moved === 1 ? '' : 's'} reassigned)` : ''; } +/** + * The skipped-openings notice, or `null` when nothing was skipped. + * + * `openings.skipped` (wall-opening-reassign.ts) counts doors/windows whose + * placement could not be interpreted — they stay attached to the now-tombstoned + * source wall rather than either half, so they can end up orphaned. + * + * Lives beside {@link formatOpeningReassignSuffix} and is exported for the SAME + * reason both are shared rather than inlined: a wall split commits from TWO + * call sites — a click (`handleSelectionClick` above) and a typed distance + * (`SplitNumericInput.tsx`) — and #3023 fixed only the click one, leaving the + * numeric path still reading `toLeft`/`toRight` and discarding `skipped`. + * Anything either toast says about openings belongs here, so the two paths + * cannot report differently on the same summary again. + */ +export function formatSkippedOpeningsNotice(op: { skipped: number }): string | null { + if (op.skipped <= 0) return null; + return `${op.skipped} opening${op.skipped === 1 ? '' : 's'} could not be reassigned and may need manual repositioning`; +} + /** Signed 2D polygon area via the shoelace formula. */ function polygonArea2D(points: Array<[number, number]>): number { if (points.length < 3) return 0; diff --git a/apps/viewer/src/components/viewer/tools/SplitNumericInput.skipNotice.test.tsx b/apps/viewer/src/components/viewer/tools/SplitNumericInput.skipNotice.test.tsx new file mode 100644 index 0000000000..d301bbc3cf --- /dev/null +++ b/apps/viewer/src/components/viewer/tools/SplitNumericInput.skipNotice.test.tsx @@ -0,0 +1,107 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ + +/** + * A wall split commits from TWO places, and #3023 only fixed one. + * + * `handleSelectionClick` (selectionHandlers.ts) is the click path, and + * `selectionHandlers.wallSplitToast.test.ts` pins the skipped-openings notice + * there. `SplitNumericInput.tsx` is the other one — type a distance, press + * Enter — and it kept its own inline copy of the "(N openings reassigned)" + * string with no reading of `openings.skipped` at all, so a skip stayed + * exactly as silent on that path as it had been everywhere before #3023. + * + * These tests drive the real component's Enter-to-commit handler against a + * stubbed `splitWallAtDistance` and assert on the toasts that reach the user, + * both directions: a notice when `skipped > 0`, silence when it is 0. + */ + +import '@/test/setup-dom.js'; +import { describe, it, beforeEach, afterEach } from 'node:test'; +import assert from 'node:assert/strict'; +import { act } from 'react'; +import { useViewerStore } from '@/store'; +import { toast } from '@/components/ui/toast'; +import { render } from '@/test/render'; +import { SplitNumericInput } from './SplitNumericInput'; + +const originalInfo = toast.info; +const originalSuccess = toast.success; +let infoCalls: string[]; +let successCalls: string[]; + +function splitResult(openings: { toLeft: number; toRight: number; skipped: number }) { + return () => ({ + ok: true as const, + left: { expressId: 1, globalId: 101 }, + right: { expressId: 2, globalId: 102 }, + openings, + }); +} + +function seedStore(openings: { toLeft: number; toRight: number; skipped: number }) { + useViewerStore.setState({ + activeTool: 'split', + splitMode: 'aiming', + splitHoverPoint: [0, 0, 0], + splitHoverDistance: 1.5, + splitHoverLength: 3, + splitTargetModelId: 'm1', + splitTargetExpressId: 42, + cameraCallbacks: { projectToScreen: () => ({ x: 100, y: 100 }) }, + splitWallAtDistance: splitResult(openings), + clearSplitHover: () => {}, + setSelectedEntityId: () => {}, + } as never); +} + +/** Press Enter on the panel's numeric input — the commit gesture. */ +function pressEnter(container: HTMLElement) { + const input = container.querySelector('input'); + assert.ok(input, 'expected the split numeric input to render'); + act(() => { + input!.dispatchEvent( + new KeyboardEvent('keydown', { key: 'Enter', bubbles: true, cancelable: true }), + ); + }); +} + +describe('SplitNumericInput: the typed-distance path reports skipped openings too', () => { + beforeEach(() => { + infoCalls = []; + successCalls = []; + (toast as { info: (m: string) => void }).info = (m: string) => infoCalls.push(m); + (toast as { success: (m: string) => void }).success = (m: string) => successCalls.push(m); + }); + + afterEach(() => { + (toast as { info: (m: string) => void }).info = originalInfo; + (toast as { success: (m: string) => void }).success = originalSuccess; + }); + + it('surfaces the skipped-openings notice, matching the click path word for word', () => { + seedStore({ toLeft: 1, toRight: 0, skipped: 2 }); + const container = render(); + pressEnter(container); + + assert.equal(successCalls.length, 1, 'expected the wall-split success toast'); + assert.ok( + successCalls[0].includes('(1 opening reassigned)'), + `expected the reassigned suffix, got: ${JSON.stringify(successCalls)}`, + ); + assert.ok( + infoCalls.some((m) => m.includes('2 openings could not be reassigned')), + `expected a skipped-openings notice, got: ${JSON.stringify(infoCalls)}`, + ); + }); + + it('stays silent when nothing was skipped', () => { + seedStore({ toLeft: 1, toRight: 1, skipped: 0 }); + const container = render(); + pressEnter(container); + + assert.equal(successCalls.length, 1, 'expected the wall-split success toast'); + assert.equal(infoCalls.length, 0, 'expected no skipped-openings notice when skipped === 0'); + }); +}); diff --git a/apps/viewer/src/components/viewer/tools/SplitNumericInput.tsx b/apps/viewer/src/components/viewer/tools/SplitNumericInput.tsx index d6e3229aea..3c1e883560 100644 --- a/apps/viewer/src/components/viewer/tools/SplitNumericInput.tsx +++ b/apps/viewer/src/components/viewer/tools/SplitNumericInput.tsx @@ -33,6 +33,10 @@ import { useEffect, useRef, useState, type ChangeEvent, type KeyboardEvent } from 'react'; import { useViewerStore } from '@/store'; import { toast } from '@/components/ui/toast'; +import { + formatOpeningReassignSuffix, + formatSkippedOpeningsNotice, +} from '@/components/viewer/selectionHandlers'; const ACCENT = '#a855f7'; // purple-500 const PANEL_OFFSET_PX = 32; @@ -88,12 +92,14 @@ export function SplitNumericInput() { if (wallTry.ok) { clearSplitHover(); setSelectedEntityId(wallTry.right.globalId); - const op = wallTry.openings; - const opSummary = - op.toLeft + op.toRight > 0 - ? ` (${op.toLeft + op.toRight} opening${op.toLeft + op.toRight === 1 ? '' : 's'} reassigned)` - : ''; - toast.success(`Wall split${opSummary} — Ctrl+Z to undo`); + // Same two notices the click path raises (selectionHandlers.ts) — a + // typed distance and a click commit the SAME `splitWallAtDistance`, so + // they must report the same summary. #3023 surfaced `openings.skipped` + // on the click path only, leaving this one silently dropping it; both + // strings now come from the shared formatters so they cannot drift. + toast.success(`Wall split${formatOpeningReassignSuffix(wallTry.openings)} — Ctrl+Z to undo`); + const skippedNotice = formatSkippedOpeningsNotice(wallTry.openings); + if (skippedNotice) toast.info(skippedNotice); return; } const linearTry = splitLinearElementAtDistance(splitTargetModelId, splitTargetExpressId, distance); From c6187aeaf7290bcb3ff876efccc861cdc14de37d Mon Sep 17 00:00:00 2001 From: Petru Conduraru Date: Sat, 22 Aug 2026 14:21:54 +0300 Subject: [PATCH 2/2] fix(viewer): report skipped openings on the typed-distance wall split too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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. --- .changeset/wall-split-notices-one-emitter.md | 9 ++ .../components/viewer/selectionHandlers.ts | 52 ++------- .../selectionHandlers.wallSplitToast.test.ts | 21 ++-- .../SplitNumericInput.skipNotice.test.tsx | 107 ----------------- .../viewer/tools/SplitNumericInput.tsx | 18 +-- .../SplitNumericInput.wallSplitToast.test.tsx | 108 ++++++++++++++++++ .../src/components/viewer/wallSplitNotice.ts | 79 +++++++++++++ 7 files changed, 224 insertions(+), 170 deletions(-) create mode 100644 .changeset/wall-split-notices-one-emitter.md delete mode 100644 apps/viewer/src/components/viewer/tools/SplitNumericInput.skipNotice.test.tsx create mode 100644 apps/viewer/src/components/viewer/tools/SplitNumericInput.wallSplitToast.test.tsx create mode 100644 apps/viewer/src/components/viewer/wallSplitNotice.ts diff --git a/.changeset/wall-split-notices-one-emitter.md b/.changeset/wall-split-notices-one-emitter.md new file mode 100644 index 0000000000..2b35ba0062 --- /dev/null +++ b/.changeset/wall-split-notices-one-emitter.md @@ -0,0 +1,9 @@ +--- +"@ifc-lite/viewer": patch +--- + +Report openings a wall split could not reassign on the typed-distance path too, not only the click path. + +A wall split commits from two places, and both call the same `MutationSlice.splitWallAtDistance`, so both receive the same `openings.skipped` count — openings that stay attached to the source wall the split has just tombstoned rather than moving to either half, and can therefore end up orphaned. #3023 taught only the canvas click handler (`selectionHandlers.ts`) to surface that count. The Split tool's numeric-distance panel (`tools/SplitNumericInput.tsx`) kept its own inlined copy of the "(N openings reassigned)" wording, read only `toLeft`/`toRight`, and never looked at `skipped` at all — so committing the identical split by typing a distance instead of clicking silently dropped the warning that clicking showed. + +Both notices now come from a single emitter, `notifyWallSplit` in the new `wallSplitNotice.ts`, which both call sites invoke instead of composing toasts themselves. An emitter rather than a shared formatter is the point: a formatter is still something a call site can neglect to call, which is exactly how these two paths came apart. The module imports nothing but the toast surface, so announcing a split does not drag `selectionHandlers.ts`'s store, geometry and measurement imports into the panel. Both paths are now pinned by tests asserting the full toast strings, in both directions — the warning when `skipped > 0`, and silence when it is 0. diff --git a/apps/viewer/src/components/viewer/selectionHandlers.ts b/apps/viewer/src/components/viewer/selectionHandlers.ts index 088407fcba..2657785279 100644 --- a/apps/viewer/src/components/viewer/selectionHandlers.ts +++ b/apps/viewer/src/components/viewer/selectionHandlers.ts @@ -13,6 +13,7 @@ import { useViewerStore } from '@/store'; import { fromGlobalIdFromModels, toGlobalIdFromModels } from '@/store/globalId'; import { pointInPolygon } from '@/lib/polygon-clip'; import { toast } from '@/components/ui/toast'; +import { notifyWallSplit } from './wallSplitNotice.js'; import { raycastForPolylinePoint, isNearPolylineStart, isDuplicateClickPoint, } from './measureHandlers.js'; @@ -158,19 +159,13 @@ export async function handleSelectionClick(ctx: MouseHandlerContext, e: MouseEve if (wallTry.ok) { state.clearSplitHover(); state.setSelectedEntityId(wallTry.right.globalId); - // Mention opening reassignment in the toast only when it - // happened — silence is preferable to "0 openings moved" - // for a wall with no doors / windows. - toast.success(`Wall split${formatOpeningReassignSuffix(wallTry.openings)} — Ctrl+Z to undo`); - // `openings.skipped` (wall-opening-reassign.ts) counts doors/windows - // whose placement we couldn't interpret — they stay attached to the - // now-tombstoned source wall rather than either half, so they can - // end up orphaned. Populated when a placement chain fails to resolve - // (mutationSlice.ts), so the zero default — and this silence — is the - // common case; before #3023 only `toLeft`/`toRight` ever reached this - // toast, so a skip was silent even when it happened. - const skippedNotice = formatSkippedOpeningsNotice(wallTry.openings); - if (skippedNotice) toast.info(skippedNotice); + // Both wall-split commit paths — here and the Split tool's + // numeric-distance panel — announce the split through the same + // emitter (`wallSplitNotice.ts`), so a notice added to one cannot + // go missing from the other. That is exactly how `openings.skipped` + // stayed silent on the typed-distance path after #3023 taught this + // one to report it. + notifyWallSplit(wallTry.openings); return; } const linearTry = state.splitLinearElementAtDistance( @@ -956,37 +951,6 @@ function capitalize(s: string): string { return s.charAt(0).toUpperCase() + s.slice(1); } -/** - * The "(N openings reassigned)" suffix for the wall-split success toast. - * Pure so the wording is unit-testable without driving the full split flow - * through the store. Deliberately silent when nothing moved — see the - * call site's comment. - */ -export function formatOpeningReassignSuffix(op: { toLeft: number; toRight: number; skipped: number }): string { - const moved = op.toLeft + op.toRight; - return moved > 0 ? ` (${moved} opening${moved === 1 ? '' : 's'} reassigned)` : ''; -} - -/** - * The skipped-openings notice, or `null` when nothing was skipped. - * - * `openings.skipped` (wall-opening-reassign.ts) counts doors/windows whose - * placement could not be interpreted — they stay attached to the now-tombstoned - * source wall rather than either half, so they can end up orphaned. - * - * Lives beside {@link formatOpeningReassignSuffix} and is exported for the SAME - * reason both are shared rather than inlined: a wall split commits from TWO - * call sites — a click (`handleSelectionClick` above) and a typed distance - * (`SplitNumericInput.tsx`) — and #3023 fixed only the click one, leaving the - * numeric path still reading `toLeft`/`toRight` and discarding `skipped`. - * Anything either toast says about openings belongs here, so the two paths - * cannot report differently on the same summary again. - */ -export function formatSkippedOpeningsNotice(op: { skipped: number }): string | null { - if (op.skipped <= 0) return null; - return `${op.skipped} opening${op.skipped === 1 ? '' : 's'} could not be reassigned and may need manual repositioning`; -} - /** Signed 2D polygon area via the shoelace formula. */ function polygonArea2D(points: Array<[number, number]>): number { if (points.length < 3) return 0; diff --git a/apps/viewer/src/components/viewer/selectionHandlers.wallSplitToast.test.ts b/apps/viewer/src/components/viewer/selectionHandlers.wallSplitToast.test.ts index fa79e883df..22045b776a 100644 --- a/apps/viewer/src/components/viewer/selectionHandlers.wallSplitToast.test.ts +++ b/apps/viewer/src/components/viewer/selectionHandlers.wallSplitToast.test.ts @@ -12,11 +12,14 @@ * handler in `selectionHandlers.ts` only ever read `toLeft`/`toRight` — * `skipped` reached the toast call site and was discarded. * - * These tests exercise `formatOpeningReassignSuffix` (the pure formatter - * pulled out of the toast call) directly, and drive the real + * These tests exercise `formatOpeningReassignSuffix` (the pure formatter, + * now in `wallSplitNotice.ts` beside `notifyWallSplit` — the single emitter + * both wall-split commit paths call) directly, and drive the real * `handleSelectionClick` split branch through the store to confirm the * `toast.info(...)` skipped-openings notice actually fires when - * `openings.skipped > 0`, and does not fire when it's 0. + * `openings.skipped > 0`, and does not fire when it's 0. The other commit + * path — the Split tool's numeric-distance panel — is pinned against the + * same strings by `tools/SplitNumericInput.wallSplitToast.test.tsx`. */ import '@/test/setup-dom.js'; @@ -24,7 +27,8 @@ import { describe, it, beforeEach, afterEach } from 'node:test'; import assert from 'node:assert/strict'; import { useViewerStore } from '@/store'; import { toast } from '@/components/ui/toast'; -import { handleSelectionClick, formatOpeningReassignSuffix } from './selectionHandlers.js'; +import { handleSelectionClick } from './selectionHandlers.js'; +import { formatOpeningReassignSuffix } from './wallSplitNotice.js'; import type { MouseHandlerContext } from './mouseHandlerTypes.js'; function fakeCtx(): MouseHandlerContext { @@ -94,7 +98,10 @@ describe('wall split toast: skipped openings notice', () => { await handleSelectionClick(fakeCtx(), fakeClick()); - assert.equal(successCalls.length, 1, 'expected the wall-split success toast'); + // The exact string, not a substring: `Wall split` and `— Ctrl+Z to undo` + // are what the numeric path's test pins too, so a change to either + // path's wording has to be a deliberate change to both. + assert.deepEqual(successCalls, ['Wall split (1 opening reassigned) — Ctrl+Z to undo']); assert.ok( infoCalls.some((m) => m.includes('2 openings could not be reassigned')), `expected a skipped-openings notice, got: ${JSON.stringify(infoCalls)}`, @@ -113,7 +120,7 @@ describe('wall split toast: skipped openings notice', () => { await handleSelectionClick(fakeCtx(), fakeClick()); - assert.equal(successCalls.length, 1, 'expected the wall-split success toast'); - assert.equal(infoCalls.length, 0, 'expected no skipped-openings notice when skipped === 0'); + assert.deepEqual(successCalls, ['Wall split (2 openings reassigned) — Ctrl+Z to undo']); + assert.deepEqual(infoCalls, [], 'expected no skipped-openings notice when skipped === 0'); }); }); diff --git a/apps/viewer/src/components/viewer/tools/SplitNumericInput.skipNotice.test.tsx b/apps/viewer/src/components/viewer/tools/SplitNumericInput.skipNotice.test.tsx deleted file mode 100644 index d301bbc3cf..0000000000 --- a/apps/viewer/src/components/viewer/tools/SplitNumericInput.skipNotice.test.tsx +++ /dev/null @@ -1,107 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ - -/** - * A wall split commits from TWO places, and #3023 only fixed one. - * - * `handleSelectionClick` (selectionHandlers.ts) is the click path, and - * `selectionHandlers.wallSplitToast.test.ts` pins the skipped-openings notice - * there. `SplitNumericInput.tsx` is the other one — type a distance, press - * Enter — and it kept its own inline copy of the "(N openings reassigned)" - * string with no reading of `openings.skipped` at all, so a skip stayed - * exactly as silent on that path as it had been everywhere before #3023. - * - * These tests drive the real component's Enter-to-commit handler against a - * stubbed `splitWallAtDistance` and assert on the toasts that reach the user, - * both directions: a notice when `skipped > 0`, silence when it is 0. - */ - -import '@/test/setup-dom.js'; -import { describe, it, beforeEach, afterEach } from 'node:test'; -import assert from 'node:assert/strict'; -import { act } from 'react'; -import { useViewerStore } from '@/store'; -import { toast } from '@/components/ui/toast'; -import { render } from '@/test/render'; -import { SplitNumericInput } from './SplitNumericInput'; - -const originalInfo = toast.info; -const originalSuccess = toast.success; -let infoCalls: string[]; -let successCalls: string[]; - -function splitResult(openings: { toLeft: number; toRight: number; skipped: number }) { - return () => ({ - ok: true as const, - left: { expressId: 1, globalId: 101 }, - right: { expressId: 2, globalId: 102 }, - openings, - }); -} - -function seedStore(openings: { toLeft: number; toRight: number; skipped: number }) { - useViewerStore.setState({ - activeTool: 'split', - splitMode: 'aiming', - splitHoverPoint: [0, 0, 0], - splitHoverDistance: 1.5, - splitHoverLength: 3, - splitTargetModelId: 'm1', - splitTargetExpressId: 42, - cameraCallbacks: { projectToScreen: () => ({ x: 100, y: 100 }) }, - splitWallAtDistance: splitResult(openings), - clearSplitHover: () => {}, - setSelectedEntityId: () => {}, - } as never); -} - -/** Press Enter on the panel's numeric input — the commit gesture. */ -function pressEnter(container: HTMLElement) { - const input = container.querySelector('input'); - assert.ok(input, 'expected the split numeric input to render'); - act(() => { - input!.dispatchEvent( - new KeyboardEvent('keydown', { key: 'Enter', bubbles: true, cancelable: true }), - ); - }); -} - -describe('SplitNumericInput: the typed-distance path reports skipped openings too', () => { - beforeEach(() => { - infoCalls = []; - successCalls = []; - (toast as { info: (m: string) => void }).info = (m: string) => infoCalls.push(m); - (toast as { success: (m: string) => void }).success = (m: string) => successCalls.push(m); - }); - - afterEach(() => { - (toast as { info: (m: string) => void }).info = originalInfo; - (toast as { success: (m: string) => void }).success = originalSuccess; - }); - - it('surfaces the skipped-openings notice, matching the click path word for word', () => { - seedStore({ toLeft: 1, toRight: 0, skipped: 2 }); - const container = render(); - pressEnter(container); - - assert.equal(successCalls.length, 1, 'expected the wall-split success toast'); - assert.ok( - successCalls[0].includes('(1 opening reassigned)'), - `expected the reassigned suffix, got: ${JSON.stringify(successCalls)}`, - ); - assert.ok( - infoCalls.some((m) => m.includes('2 openings could not be reassigned')), - `expected a skipped-openings notice, got: ${JSON.stringify(infoCalls)}`, - ); - }); - - it('stays silent when nothing was skipped', () => { - seedStore({ toLeft: 1, toRight: 1, skipped: 0 }); - const container = render(); - pressEnter(container); - - assert.equal(successCalls.length, 1, 'expected the wall-split success toast'); - assert.equal(infoCalls.length, 0, 'expected no skipped-openings notice when skipped === 0'); - }); -}); diff --git a/apps/viewer/src/components/viewer/tools/SplitNumericInput.tsx b/apps/viewer/src/components/viewer/tools/SplitNumericInput.tsx index 3c1e883560..6040a7dd03 100644 --- a/apps/viewer/src/components/viewer/tools/SplitNumericInput.tsx +++ b/apps/viewer/src/components/viewer/tools/SplitNumericInput.tsx @@ -33,10 +33,7 @@ import { useEffect, useRef, useState, type ChangeEvent, type KeyboardEvent } from 'react'; import { useViewerStore } from '@/store'; import { toast } from '@/components/ui/toast'; -import { - formatOpeningReassignSuffix, - formatSkippedOpeningsNotice, -} from '@/components/viewer/selectionHandlers'; +import { notifyWallSplit } from '../wallSplitNotice.js'; const ACCENT = '#a855f7'; // purple-500 const PANEL_OFFSET_PX = 32; @@ -92,14 +89,11 @@ export function SplitNumericInput() { if (wallTry.ok) { clearSplitHover(); setSelectedEntityId(wallTry.right.globalId); - // Same two notices the click path raises (selectionHandlers.ts) — a - // typed distance and a click commit the SAME `splitWallAtDistance`, so - // they must report the same summary. #3023 surfaced `openings.skipped` - // on the click path only, leaving this one silently dropping it; both - // strings now come from the shared formatters so they cannot drift. - toast.success(`Wall split${formatOpeningReassignSuffix(wallTry.openings)} — Ctrl+Z to undo`); - const skippedNotice = formatSkippedOpeningsNotice(wallTry.openings); - if (skippedNotice) toast.info(skippedNotice); + // Same notices as the canvas click path — a split committed by typing + // a distance is the same edit on the same `splitWallAtDistance` result, + // including the warning when openings could not be reassigned + // (`openings.skipped`), which this path dropped until #3074. + notifyWallSplit(wallTry.openings); return; } const linearTry = splitLinearElementAtDistance(splitTargetModelId, splitTargetExpressId, distance); diff --git a/apps/viewer/src/components/viewer/tools/SplitNumericInput.wallSplitToast.test.tsx b/apps/viewer/src/components/viewer/tools/SplitNumericInput.wallSplitToast.test.tsx new file mode 100644 index 0000000000..0e0483ff96 --- /dev/null +++ b/apps/viewer/src/components/viewer/tools/SplitNumericInput.wallSplitToast.test.tsx @@ -0,0 +1,108 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ + +/** + * A wall split can be committed from TWO places: the canvas click handler + * (`selectionHandlers.ts`, covered by `selectionHandlers.wallSplitToast.test.ts`) + * and this panel's Cut button / Enter key. Both call the same + * `MutationSlice.splitWallAtDistance`, so both see the same `openings.skipped` + * count — openings that stay attached to the source wall the split has just + * tombstoned, and can end up orphaned. + * + * #3023 taught only the click handler to surface that count, leaving this panel + * with its own inlined copy of the success wording and no warning at all, so + * typing a distance instead of clicking hid a data problem that clicking + * reported. These tests drive the real panel through the real store and assert + * the EXACT strings the click path's test asserts, so the two commit paths + * cannot come apart again: against the pre-fix panel `toast.info` is never + * called. + */ + +import '@/test/setup-dom.js'; +import { afterEach, beforeEach, describe, it } from 'node:test'; +import assert from 'node:assert/strict'; +import { useViewerStore } from '@/store'; +import { toast } from '@/components/ui/toast'; +import { render, cleanup, click } from '@/test/render'; +import { SplitNumericInput } from './SplitNumericInput.js'; + +/** The panel's commit control. Labelled "Cut" — the Enter key runs the same `commitAt`. */ +function cutButton(container: HTMLElement): HTMLButtonElement { + const button = Array.from(container.querySelectorAll('button')).find( + (b) => b.textContent?.trim() === 'Cut', + ); + assert.ok(button, 'expected the panel to render its Cut button'); + return button as HTMLButtonElement; +} + +/** Stub `splitWallAtDistance` with a successful split reporting `openings`. */ +function seedSplit(openings: { toLeft: number; toRight: number; skipped: number }) { + useViewerStore.setState({ + splitWallAtDistance: () => ({ + ok: true, + left: { expressId: 1, globalId: 101 }, + right: { expressId: 2, globalId: 102 }, + openings, + }), + } as unknown as Partial>); +} + +describe('SplitNumericInput: wall-split notices', () => { + const original = { + splitWallAtDistance: useViewerStore.getState().splitWallAtDistance, + info: toast.info, + success: toast.success, + }; + let infoCalls: string[]; + let successCalls: string[]; + + beforeEach(() => { + infoCalls = []; + successCalls = []; + (toast as { info: (m: string) => void }).info = (m: string) => infoCalls.push(m); + (toast as { success: (m: string) => void }).success = (m: string) => successCalls.push(m); + useViewerStore.setState({ + activeTool: 'split', + splitMode: 'aiming', + splitHoverPoint: [0, 0, 0], + splitHoverDistance: 1.5, + splitHoverLength: 3, + splitTargetModelId: 'm1', + splitTargetExpressId: 42, + cameraCallbacks: { projectToScreen: () => ({ x: 10, y: 10 }) }, + clearSplitHover: () => {}, + setSelectedEntityId: () => {}, + } as unknown as Partial>); + }); + + afterEach(() => { + cleanup(); + (toast as { info: (m: string) => void }).info = original.info; + (toast as { success: (m: string) => void }).success = original.success; + useViewerStore.setState({ splitWallAtDistance: original.splitWallAtDistance }); + }); + + it('warns about openings the split could not reassign', () => { + seedSplit({ toLeft: 1, toRight: 0, skipped: 2 }); + + const container = render(); + click(cutButton(container)); + + assert.deepEqual(successCalls, ['Wall split (1 opening reassigned) — Ctrl+Z to undo']); + assert.ok( + infoCalls.some((m) => m.includes('2 openings could not be reassigned')), + `expected a skipped-openings notice, got: ${JSON.stringify(infoCalls)}`, + ); + }); + + it('stays silent about skipped openings when none were skipped', () => { + seedSplit({ toLeft: 1, toRight: 1, skipped: 0 }); + + const container = render(); + click(cutButton(container)); + + assert.deepEqual(successCalls, ['Wall split (2 openings reassigned) — Ctrl+Z to undo']); + assert.deepEqual(infoCalls, []); + }); +}); diff --git a/apps/viewer/src/components/viewer/wallSplitNotice.ts b/apps/viewer/src/components/viewer/wallSplitNotice.ts new file mode 100644 index 0000000000..f2a72e9963 --- /dev/null +++ b/apps/viewer/src/components/viewer/wallSplitNotice.ts @@ -0,0 +1,79 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ + +/** + * The user-facing notices a committed wall split emits. + * + * `MutationSlice.splitWallAtDistance` is reached from TWO places — the canvas + * click handler (`selectionHandlers.ts`) and the Split tool's numeric-distance + * panel (`tools/SplitNumericInput.tsx`) — and both must report the same split + * the same way. They previously each inlined their own copy of the + * "(N openings reassigned)" wording, and #3023 taught only the click handler to + * also surface `openings.skipped`, so committing the identical split by typing + * a distance instead of clicking silently dropped that warning. + * + * This module is the single definition of both notices. Call + * {@link notifyWallSplit} rather than composing the toasts at a call site: a + * shared formatter is still one a new call site can forget to call, which is + * precisely how the two paths came apart. A shared EMITTER cannot be half-used. + * + * It deliberately imports nothing but the toast surface, so a call site does + * not take on `selectionHandlers.ts`'s store, geometry and measurement imports + * just to announce a split. + */ + +import { toast } from '@/components/ui/toast'; + +/** + * `OpeningReassignSummary` as far as the notices care: how many of the source + * wall's openings moved to each half, and how many were left on neither. + * + * Produced by `reassignWallOpenings` (`@/lib/wall-opening-reassign.ts`) and + * forwarded verbatim by `splitWallAtDistance` + * (`store/slices/mutationSlice.ts:1981`), which also substitutes an all-zero + * summary when either half's placement chain does not resolve and the + * reassignment is therefore never attempted. + */ +export interface OpeningReassignCounts { + toLeft: number; + toRight: number; + skipped: number; +} + +/** + * The "(N openings reassigned)" suffix for the wall-split success toast. + * Pure so the wording is unit-testable without driving the full split flow + * through the store. Deliberately silent when nothing moved: "0 openings + * moved" is noise for a wall with no doors or windows. + */ +export function formatOpeningReassignSuffix(op: OpeningReassignCounts): string { + const moved = op.toLeft + op.toRight; + return moved > 0 ? ` (${moved} opening${moved === 1 ? '' : 's'} reassigned)` : ''; +} + +/** + * Announce a wall split that has already been committed. + * + * Always emits the success toast. Additionally warns when + * `openings.skipped > 0`: those openings stay attached to the source wall the + * split has just tombstoned rather than moving to either half, so they can end + * up orphaned. + * + * `skipped` is incremented at twelve distinct sites in + * `@/lib/wall-opening-reassign.ts` (lines 123-203), only one of which 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 somewhere other than the source + * wall is skipped on purpose, because rewriting its parent placement would + * teleport it. Zero is the ordinary outcome, which is why this stays quiet + * then; it must not be quiet on ONE of the two commit paths when it is not. + */ +export function notifyWallSplit(op: OpeningReassignCounts): void { + toast.success(`Wall split${formatOpeningReassignSuffix(op)} — Ctrl+Z to undo`); + if (op.skipped > 0) { + toast.info( + `${op.skipped} opening${op.skipped === 1 ? '' : 's'} could not be reassigned and may need manual repositioning`, + ); + } +}