mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-30 11:49:16 +00:00
claude[bot]
@@ -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`.
|
||||
Reference in New Issue
Block a user