diff --git a/docs/reviews/CODE-REVIEW-229-r1.md b/docs/reviews/CODE-REVIEW-229-r1.md new file mode 100644 index 00000000..9ed211a3 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-229-r1.md @@ -0,0 +1,282 @@ +# CODE-REVIEW-229-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/229 +- **Спецификация:** `docs/specs/229-merge-collinear-partitions.md` (зелёное ревью r3, `docs/reviews/SPEC-REVIEW-229-r3.md`) +- **Ветка:** `issue/229-merge-collinear-partitions` +- **Коммиты разбора:** `e6be43b` (feat), `50a00e4` (docs: скриншоты) +- **Заход:** r1 · блокирующих циклов израсходовано 0/4 до этого вердикта +- **Диапазон:** `git diff origin/dev...HEAD` + +## Скоуп + +Новый чистый модуль `src/wall-merge.ts` (`mergeCollinearPartitions`, +`junctionAt`, `applyOpeningMoves`) плюс два места вызова: `_finishWallChain` → +`_mergeSpacePartitions` в `src/houseplan-card.ts` (слияние своей цепочки при +рисовании, §8.6) и `optimizePlans` в `src/plan-optimizer.ts` (слияние всего +пространства при «Оптимизировать планы», §4.2). Плюс i18n строка счётчика, +changelog RU+EN, `docs/USER-GUIDE.ru.md`, юниты, новый смок +`demo/smoke_wall_chain_merge.mjs`, семь записей мутационного гейта. + +## Как проверялось + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | чисто | +| Unit | `npm test` | 1003 pass / 0 fail | +| Build + сверка бандлов | `npm run build && cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js && cmp dist/houseplan-card.js demo/srv/assets/houseplan-card.js` | идентичны | +| Смок (AC1/§8.6, названный автором) | `node demo/smoke_wall_chain_merge.mjs` | OK, но см. High-2 — смок не способен обнаружить найденный дефект (объяснение ниже) | +| Смежные смоки (пересчёт проёмов/стыков, задетые diff'ом) | `node demo/smoke_partition_openings.mjs`, `node demo/smoke_wall_junctions.mjs`, `node demo/smoke_optimize_coordinate_canonicalization.mjs` | все OK, регрессий не нашёл | +| Собственная проверка через реальный клик-путь редактора (не смок из репозитория, вспомогательный скрипт вне репозитория, тот же `demo/serve.mjs`) | см. воспроизведение High-1 и High-2 ниже | обнаружил 2 дефекта | +| Прямой вызов `optimizePlans` из `test-build/plan-optimizer.js` с комнатой | см. воспроизведение High-1 | обнаружил дефект | + +**Не прогонял и почему:** `npm run golden:verify` — спецификация §15 явно +говорит «golden не затрагивается: видимый результат на плане не меняется», +diff подтверждает — правки внутреннего представления, не рендера. +`python -m pytest tests_backend` — diff не касается `custom_components/**/*.py`. +Полный набор из 127 браузерных смоков — diff и AC называют одну подсистему +(перегородки/проёмы/оптимизатор), не весь продукт; прогнал названный автором +смок плюс три соседних по геометрии проёмов и стыков. Performance-профили — в +AC не названы, спецификация §10 заявляет отсутствие влияния на кадр рендера +достаточно правдоподобно (разовый проход по десяткам записей), не тронуто. + +## Находки + +### High-1 — «узел на стыке с комнатой» не работает: слияние проходит сквозь T-стык к стене комнаты (§8.2, AC2) + +И `src/houseplan-card.ts:6556-6559` (`_mergeSpacePartitions`), и +`src/plan-optimizer.ts:506-509` (`optimizePlans`) строят `geometry.roomPolygons` +так: + +```ts +roomPolygons: rooms + .map((room) => roomPoly(room)) + .filter((poly): poly is number[][] => !!poly) + .map((poly) => poly.map((point) => [point[0] / NORM_W, point[1] / NORM_W])), +``` + +`roomPoly(room)` в обоих местах вызывается на **уже сохранённом** `space.rooms` +(не на де-нормализованной модели из `spaceModels()`), а `room.poly` хранится +**уже нормализованным** к `[0,1]` — это видно из места, где полигон комнаты +записывается: `houseplan-card.ts:12638`, `poly: verts.map((p) => [p[0] / NORM_W, p[1] / NORM_W])`. +Тот же `space.partitions`, переданный на предыдущей строке в +`mergeCollinearPartitions(space.partitions || [], …)`, используется БЕЗ какой-либо +конверсии — потому что он в той же нормализованной шкале. Тот же файл +`plan-optimizer.ts` двумя строками раньше (492-495) явно демонстрирует +обратное преобразование: `degradeWalls(walls, space.rooms || [], GRID_STEP_N, 1, +cuts.map((c) => [c[0] / NORM_W, …]))` — здесь `cuts` (сырые, в масштабе +NORM_W) **делятся** на `NORM_W`, чтобы сравняться по шкале с `space.rooms`, +используемым без конверсии. То есть в этом же файле уже задокументировано, +что `space.rooms` — нормализованная шкала; новый код применяет к ней ещё одно +деление, получая координаты около `~0.0001` — на три порядка меньше типичных +координат перегородок (`~0.1…0.9`). Практический эффект: полигон комнаты +после этого находится в точке, неотличимой от начала координат, и +`distToSegment`/`junctionAt` никогда не находят реального совпадения. + +**Воспроизведено исполнением, дважды:** + +1. Прямой вызов `optimizePlans` (`test-build/plan-optimizer.js`, тот же билд, + что использует `npm test`) с комнатой, чья верхняя сторона идёт по + `y=0.4` от `x=0.1` до `x=0.5`, и двумя коллинеарными перегородками + `[0.1,0.4]→[0.3,0.4]` и `[0.3,0.4]→[0.5,0.4]` (стык — ровно середина + стороны комнаты, canonical T-стык из AC2 и `docs/specs/141-wall-junctions.md`): + `result.report.partitionsMerged === 1`, две записи схлопнулись в одну — + AC2 в этой части ложно-зелёный. +2. То же в браузере через реальный редактор: комната с тем же полигоном, + три клика вдоль её верхней стороны (`(100,400)→(300,400)→(500,400)` в + координатах канвы), `_activateMarkupTool('select')` для завершения цепочки + — итог `space.partitions.length === 1` вместо ожидаемых двух. + +Юнит-тесты `test/wall-merge.test.mjs` (`a room side keeps the node…`) этот +дефект не ловят, потому что вызывают `mergeCollinearPartitions` напрямую с +полигоном комнаты, уже заданным в масштабе, совпадающем с перегородками, — +интеграционное лишнее деление там не участвует. `test/plan-optimizer.test.mjs` +тоже не ловит: все три новых теста для AC7 используют `rooms: []`. Мутант +`junction-checks-room-vertices-only` в `scripts/mutation-gate.mjs` целится в +`src/wall-merge.ts` напрямую тем же юнитом — интеграционную обвязку не +проверяет. + +**Почему в скоупе и почему High.** Это ровно тот случай T-стыка к середине +комнатной стены, который потребовал двух раундов ревью ТЗ (M2 → r2, находка +r2 → r3) и явно назван нормативным в §8.2/AC2 как обязательный к сохранению. +В реальной интеграции защита не работает: любая перегородка, упирающаяся в +середину стены комнаты, теряет узел при рисовании или при «Оптимизировать +планы», как только на этом стыке есть ещё один коллинеарный отрезок той же +толщины. Правка не требует пересмотра контракта — убрать лишнее `/NORM_W` в +обоих местах (`houseplan-card.ts:6559`, `plan-optimizer.ts:509`). + +### High-2 — слияние «во что упёрлась цепочка» не работает через реальный клик-путь (§8.6) + +`_finishWallChain` (`houseplan-card.ts:6595-6616`) вызывает +`this._mergeSpacePartitions(sp, drawnIds)` (строка 6612) **до** того, как +активный черновик убирается из `sp.room_drafts` (фильтрация — строки +6613-6616, ПОСЛЕ вызова слияния). Каждый клик при рисовании стены сохраняет +прогресс через `_persistActiveDraftSegment` (`houseplan-card.ts:7420-7443`) — +в `sp.room_drafts` появляется запись с `id = this._activeDraftId` и +`points = this._path`, то есть полный путь **именно той цепочки, которая +сейчас завершается**. `_mergeSpacePartitions` строит `geometry.draftEnds` из +`sp.room_drafts` безусловно (строки 6561-6564), не исключая +`this._activeDraftId` — в отличие от уже существующего в этом же файле +паттерна для точно такой же проблемы: `buildPlanSnapGeometry` +(`plan-snap-overlay.ts:150-151`) явно пропускает активный черновик +(`if (draft.id === options.activeDraftId) continue;`), потому что черновик, +который рисуется прямо сейчас, не является «другим» сохранённым черновиком, +на который можно вернуться. + +Эффект: если новая цепочка начинается или заканчивается ровно там, где +кончается уже существующая коллинеарная перегородка той же толщины (самый +естественный сценарий продолжения стены во второй сессии рисования), точка +стыка совпадает с собственным `draftEnds` черновика, который вот-вот +исчезнет, — `junctionAt` находит «причину» и слияние не происходит. + +**Воспроизведено исполнением через реальный клик-путь (`_markupClick`, не +через прямое присваивание `_path`):** нарисована стена `(200,500)→(300,500)`, +толщина 15, завершена сменой инструмента на `select` (`sp.partitions.length +=== 1`, `sp.room_drafts` пуст). Затем во второй сессии рисования — стена +`(300,500)→(420,500)`, та же толщина, начинающаяся ровно в конце первой. +После завершения: `sp.partitions.length === 2`, коллинеарные отрезки той же +толщины с общим концом не срослись. + +**Смок `demo/smoke_wall_chain_merge.mjs` не ловит это** ровно потому, что +его третий чек (`chainMergesIntoTheWallItTouches`) задаёт `c._path` +присваиванием, минуя `_persistActiveDraftSegment` — `this._activeDraftId` +остаётся `null` весь тест, `sp.room_drafts` никогда не заполняется, и путь, +где лежит дефект, не исполняется вовсе. Смок проверяет «умеет ли модуль +слияния сращивать через существующую стену», а не «работает ли это при +рисовании» — то самое требование «тест умеет падать» (AGENTS.md/PROCESS.md +§2.7) в этом месте не выполнено, хотя формально смок называется в AC и +хендоффе как покрывающий именно этот сценарий. + +**Почему в скоупе и почему High.** §8.6 прямо формулирует это как часть +контракта («слияние затрагивает… сегменты самой цепочки и те существующие +перегородки, с которыми она имеет общий конец»), это же явно заявлено в +сообщении коммита («Рисование сращивает только свою цепочку и то, чего она +коснулась») и в самом смоке. В реальном использовании эта часть контракта не +работает для самого частого случая — продолжения стены в новой сессии +рисования. Фикс мелкий и уже есть образец в этом же файле: не включать +`this._activeDraftId` в `draftEnds` (например, отфильтровать перед вызовом +`_mergeSpacePartitions`, либо — надёжнее — переставить фильтрацию +`sp.room_drafts` перед вызовом слияния, чтобы `_mergeSpacePartitions` вообще +не видел запись, которая всё равно исчезнет). + +### Low — комментарий про допуски (`EPS_ANGLE`) не соответствует реализации, поведение корректно + +`src/wall-merge.ts:20-23`: `EPS_ANGLE` документирован как «a fraction of one +grid pitch», но в `mergeCollinearPartitions` (строка 157) используется +напрямую, без умножения на `pitch`: `const angle = EPS_ANGLE;`. Это +математически оправдано — модуль векторного произведения единичных +направляющих (`cross`) безразмерен и не должен масштабироваться шагом сетки +(в отличие от `EPS_JOIN`, который действительно домножается на `pitch`, +строка 156). Поведение корректно и не противоречит AC5 (проверено юнитом +«tolerance forgives float noise and refuses a real gap»); расхождение чисто +документальное — комментарий вводит в заблуждение, будто оба допуска +масштабируются одинаково. Не блокирует, снимаю с записью: автор может +поправить формулировку комментария (например, «expressed as a plain +tolerance on the sine of the angle, independent of pitch») при следующей +правке этого файла, отдельного цикла ради одной строки комментария не +считаю оправданным. + +## Что проверено и корректно + +- **AC1** (пять кликов по прямой → одна запись): доказано юнитом + (`test/wall-merge.test.mjs`, «a straight chain… collapses») и смоком + (`straightRunIsOneRecord`), дополнительно перепроверено собственным + прогоном через реальный клик-путь — не задето находками выше, поскольку + внутренние стыки цепочки не попадают в `draftEnds` (только первая и + последняя точка всего пути). +- **AC2**, случаи «третья перегородка», «колонна», «конец черновика в + стороне» — юниты корректны; интеграционный масштаб для `columns` и + `draftEnds` (кроме самоссылки из High-2) выставлен верно — `wall_columns[].center` + хранится нормализованным (`houseplan-card.ts:7579`) и передаётся без + лишней конверсии, `room_drafts[].points` тоже (`houseplan-card.ts:7434`). + Сломан только под-случай «ребро комнаты» (High-1). +- **AC3** (проём не двигается, оба представления — `host.t` и материализованная + проекция `x/y/angle`): юниты `wall-merge.test.mjs` и + `plan-optimizer.test.mjs` доказывают на прямом и на развернувшемся стыке; + тест сформулирован так, что красен при пересчёте только `host.t` без + проекции (проверено чтением реализации `applyOpeningMoves` — вызывает + `materializePartitionOpening` после `resolvePartitionOpeningCompat` для + каждого перемещённого проёма, `wall-merge.ts:238-262`) — доказательство + соответствует AC3 буквально. +- **AC4** (разная толщина не сращивается): юнит корректен, `pairAt` сравнивает + `cm` строго (`wall-merge.ts:132`). +- **AC5** (допуски прощают ULP-шум, не прощают разведённое): юнит корректен; + см. Low-находку про `EPS_ANGLE` — не влияет на результат. +- **AC6** (детерминизм, независимость от порядка входа): юнит проверяет + прямой/обратный/перетасованный порядок, корректно; сам механизм — + канонизация направления выжившей записи лексикографически + (`wall-merge.ts:189-192`) — устраняет реальный класс бага, который автор + описывает в хендоффе (переворот `host.t` при недетерминированном + направлении); переиспользование записей `moves` при цепочке слияний + (`wall-merge.ts:203-216`) проверено чтением и юнитом «an opening survives a + chain of merges and a reversed survivor» — корректно. +- **AC7** (оптимизатор сращивает накопленное, идемпотентен): юнит + `plan-optimizer.test.mjs` проверяет оба свойства на трёх коллинеарных + отрезках плюс независимую стену в стороне; повторный прогон даёт + `partitionsMerged === 0` — корректно для случая без комнат (см. High-1 для + случая с комнатами). +- **AC8** (ничего лишнего; область слияния при рисовании ограничена + компонентой связности новой цепочки): юниты «finishing a chain does not + sweep unrelated seams» и «a chain merges into what it was drawn onto» + доказывают это для чистого модуля; мутант `chain-merge-sweeps-whole-space` + проверен применением патча (1 падение, соответствует хендоффу). Механизм + `seedIds`/расширение множества выживших id при последовательных слияниях + (`wall-merge.ts:217`) проверен чтением — корректен. Это свойство **не + противоречит** High-2: там цепочка НЕ сращивается, когда должна была бы, а + не сращивается с чем-то лишним. +- **AC9** (release-артефакты): `docs/CHANGELOG.md`/`docs/CHANGELOG.ru.md` + правлены в том же коммите `e6be43b`, что и код (`User-Visible: yes`), i18n + строка добавлена в оба языка и проверена тестом + (`test/i18n.test.mjs`), три копии бандла байт-в-байт идентичны (проверено + исполнением, `cmp` × 2). +- **Docs/`50a00e4`**: отдельный коммит `User-Visible: no`, скриншоты — автор + утверждает, что отпечаток `check-docs.mjs` был просрочен ещё на чистом + `dev` (несвязанный долг); не перепроверял это утверждение исполнением + (не относится к AC #229), содержательных изменений в кадрах сам коммит не + подразумевает. +- **Трейлеры и процесс:** оба коммита несут `Issue: #229`; `e6be43b` — + `User-Visible: yes` с изменением обоих changelog в том же коммите; `50a00e4` + — `User-Visible: no`, класс C (только `docs/images/**`, + `docs/images/screenshots.json`), корректно. `scripts/process-gate.mjs` + — не прогонял отдельно (автор указал прогон в хендоффе, офлайн-часть не + зависит от находок этого ревью). + +## Чего не проверял + +- Полный браузерный набор (127 смоков) — не запускал целиком, diff и AC не + требуют; прогнал названный автором смок плюс три смежных по геометрии + проёмов/стыков. +- `npm run golden:verify` — по спецификации видимый результат не меняется; + не перепроверял исполнением этого утверждения растеризацией. +- `python -m pytest tests_backend` — diff не касается Python. +- Производительность — не профилировал; утверждение спецификации о разовом + проходе по десяткам записей проверено чтением (`mergeCollinearPartitions` + — `O(n²)` на раунд слияния, вызывается на списке перегородок одного + пространства, не в цикле рендера). +- Обратимость через Undo/Redo слияния при рисовании (`_recordGeometry` + перед `_finishWallChain`, `houseplan-card.ts:6617`) — проверено только + чтением вызова, не отдельным сценарием отмены. +- Достоверность утверждения о просроченном отпечатке `check-docs.mjs` на + чистом `dev` в коммите `50a00e4` — принято со слов автора, не + перепроверял `git stash`/переисполнением на чистом `dev`. + +## Вспомогательные материалы + +Обе находки воспроизведены одноразовыми скриптами вне репозитория (не +коммитились, репозиторий не менялся): вызов `optimizePlans` напрямую из +`test-build/plan-optimizer.js` для High-1, и реальный клик-путь через +`demo/serve.mjs`/`page.evaluate` для обеих находок. Результаты вставлены в +текст находок буквально (значения `partitionsMerged`, `partitions.length`). + +## Вердикт + +**Красный.** Два High: критическая часть контракта §8.2 (узел на стыке с +комнатой) и §8.6 (слияние с тем, чего цепочка коснулась) не работают в +реальной интеграции, хотя специфицированы, заявлены в коммите и формально +«покрыты» смоком/юнитами, которые, как показано выше, не проверяют именно +эти пути. Обе находки — в скоупе задачи (это код, добавленный этим PR, а не +соседняя подсистема), обе чинятся точечно без пересмотра контракта: +убрать лишнее `/NORM_W` в двух местах (High-1) и не включать активный +черновик в `draftEnds`, либо переставить очистку `room_drafts` перед вызовом +слияния (High-2), плюс тест на каждый случай через реальный путь +(реальные клики / комната с полигоном), а не только через изолированный +`mergeCollinearPartitions`.