diff --git a/.changeset/wall-split-notices-one-emitter.md b/.changeset/wall-split-notices-one-emitter.md new file mode 100644 index 000000000..2b35ba006 --- /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 3a78b6ffc..265778527 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,21 +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. 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`, - ); - } + // 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( @@ -958,17 +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)` : ''; -} - /** 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 fa79e883d..22045b776 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.tsx b/apps/viewer/src/components/viewer/tools/SplitNumericInput.tsx index d6e3229ae..6040a7dd0 100644 --- a/apps/viewer/src/components/viewer/tools/SplitNumericInput.tsx +++ b/apps/viewer/src/components/viewer/tools/SplitNumericInput.tsx @@ -33,6 +33,7 @@ import { useEffect, useRef, useState, type ChangeEvent, type KeyboardEvent } from 'react'; import { useViewerStore } from '@/store'; import { toast } from '@/components/ui/toast'; +import { notifyWallSplit } from '../wallSplitNotice.js'; const ACCENT = '#a855f7'; // purple-500 const PANEL_OFFSET_PX = 32; @@ -88,12 +89,11 @@ 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 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 000000000..0e0483ff9 --- /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 000000000..f2a72e996 --- /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`, + ); + } +}