diff --git a/docs/reviews/CODE-REVIEW-132-r2.md b/docs/reviews/CODE-REVIEW-132-r2.md new file mode 100644 index 00000000..1b67c967 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-132-r2.md @@ -0,0 +1,265 @@ +# CODE-REVIEW-132-r2 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/132 +- **Связанный bug в том же scope:** #185 (закрывается тем же кодом, по решению владельца) +- **Диапазон:** `origin/dev...HEAD` на ветке `issue/132-partition-openings-v2`, + но предметно этот цикл разбирает только новый коммит с момента r1 — + `3fe0f8c fix: address partition opening review regressions` (16 файлов, + +306/−244), поверх уже принятого без изменений `9f77e3e` (r1 покрыл его целиком). +- **ТЗ:** `docs/specs/132-partition-openings.md`, зелёное ревью + `docs/reviews/SPEC-REVIEW-132-r1.md` +- **Предыдущий цикл:** `docs/reviews/CODE-REVIEW-132-r1.md` — красный, + High: 1 (`build_space_merge()` не ремапил `opening.host.id`), Medium: 3 + (#186, #187, #188, заведены отдельно, не входят в этот фикс) +- **Роль:** ревьюер кода (не автор), этап `S7-code-review` +- **Цикл:** r2/4 + +## Скоуп ревью + +Коммит `3fe0f8c` заявлен как исправление ровно High-1 из r1. Фактически несёт +два независимых изменения: + +1. **Ремап `opening.host.id`** в `build_space_merge()` + (`custom_components/houseplan/import_export.py:845-854`) — прямое исправление + High-1, плюс регрессионный тест в `tests_backend/test_ha_import_export.py`. +2. **Разделение presentation/structural снапшотов** `buildPlanSnapGeometry()` + в `src/houseplan-card.ts` (новый `_planStructuralGeometrySnapshot()`, + `_planSnapGeometrySnapshot()` восстанавливает opening-cuts, `_wallGraphSources` + переключён на структурный снапшот) плюс правки `src/plan-snap-overlay.ts` + (только докстринг), `docs/CANVAS.md`, changelog RU/EN, doc-screenshots. + +Второе не было прямо потребовано r1 (High-1 касался только backend), но +устраняет регрессию, которую r1 обнаружил и не смог объяснить (golden-diff +`plan-snap-line-gaps-dark`, зафиксирован в r1 как «Чего не проверял», не как +находка) — это восстановление поведения `_planSnapOpeningCuts()`, которое +существовало на `origin/dev` до правки #132 и было потеряно в `9f77e3e`. Не +считаю это расширением скоупа: правка находится в файлах и подсистеме, которые +сам r1 разбирал по AC5/AC6, и не добавляет пользователю ничего вне контракта +из ТЗ §11. + +Прочие 11 AC (AC1–AC4, AC6–AC12) не затронуты диапазоном `3fe0f8c` — сам файл-состав +диффа (backend id-ремап + presentation/structural split) не пересекается с их +кодовыми путями placement/lifecycle/light/HA-state/compatibility-schema/i18n, +что подтверждено построчным чтением диффа. Повторный разбор по каждому из них +не производился — r1 их разобрал, вердикт по этому циклу ограничен тем, что +изменилось. + +## Как проверялось + +Дешёвые гейты (прогнаны лично в этой сессии, точные команды и результат): + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | зелёный, без вывода | +| Unit | `npm test` | `870 passed / 0 failed`, совпадает с заявленным | +| 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` | сборка ок, обе копии побайтово совпадают, `git status --short` после сборки пуст (закоммиченный бандл идентичен свежей сборке) | + +Смоки, прогнанные локально (по diff — только затронутые этим коммитом поверхности): + +| Смок | Результат | Почему выбран | +|---|---|---| +| `node demo/smoke_partition_openings.mjs` | OK, все 12 полей `true` | использует переименованный `_planStructuralGeometrySnapshot`, регрессия по AC1/AC3/AC5 | +| `node demo/smoke_room_autoclose.mjs` | OK, все 9 полей `true` | `openingKeepsStructuralAutoClose: true` — #185 не пострадал от разделения снапшотов | +| `node demo/smoke_plan_snap_overlay.mjs` | OK, все 32 поля `true`, включая `openingGapHasNoLine: true` | прямая проверка presentation-cut; **не входил** в список из 12 смоков r1, хотя diff трогает именно эту поверхность — восполняю здесь | + +Остальные смоки из целевого набора r1 (`smoke_opening_preview`, `smoke_glow`, +`smoke_inert_openings`, `smoke_opening_binding`, `smoke_opening_tunnel_fill`, +`smoke_wall_junctions`, `smoke_unified_wall_tool`, `smoke_isometric_contract`, +`smoke_openwall`, `smoke_sun`) не перегонялись: `3fe0f8c` не меняет код на их +путях (placement/light/HA-binding/junction geometry/iso/#173 regression — +подтверждено чтением диффа), а r1 уже доказал их зелёными на предшествующем +коммите. Не весь набор из 141 — избыточно для 16-файлового фикса. + +**Backend.** В окружении этого ревью нет `homeassistant`/`.venv-backend` (то же +ограничение, что у r1). Установил `pytest`+`voluptuous` и прогнал чистое +подмножество: `python -m pytest tests_backend -q` → **139 passed** — тот же +результат, что у r1, `test_ha_import_export.py` (файл с фиксом High-1 и новым +регрессионным тестом) в этом прогоне **не участвовал** (`conftest.py` молча +игнорирует `test_ha_*.py` без `homeassistant`). + +Вместо предположения проверил через реальный CI на этом SHA (Linux, +`homeassistant` установлен): job `backend` для `3fe0f8c` — +**`completed / success`**, лог оканчивается `280 passed in 4.30s`, команда +в логе — `python -m pytest tests_backend/ -q`. Для контраста: тот же job на +предыдущем коммите `9f77e3e` (до фикса) — `completed / failure`. Это прямое +подтверждение, что регрессионный тест `test_space_merge_remaps_every_space_owned_id_and_room_link` +(теперь с `host`-фикстурой) действительно запускается на Linux CI и проходит +после фикса, а до фикса — ломался. Не «verified» без команды: команда и результат +процитированы из фактического лога джобы +(`https://github.com/Matysh/houseplan-card/actions/runs/32193375736/job/95892362527`). + +**Golden.** `job golden` на `3fe0f8c` — `completed / success`, в логе все +перечисленные сцены `passed`, включая ранее «different»/нерасследованную в r1 +`plan-snap-line-gaps-dark` — теперь **`passed`**. Это лучше, чем 60/62 у r1: +второй компонент фикса (presentation-снапшот) действительно устранил тот +golden-diff, который r1 оставил неразобранным. + +**Прочитано построчно, не исполнено:** +- `custom_components/houseplan/import_export.py:845-854` — ремап `opening.host.id` + через тот же `id_map`, что и id перегородки; порядок операций корректен: + цикл ремапа id (`:830-838`) выполняется раньше, `id_map` уже содержит + `old_partition_id → new_partition_id`, когда цикл над `openings` (`:849-854`) + читает `host.id`. +- `src/houseplan-card.ts:6156-6198` — `_planSnapGeometrySnapshot` восстановил + `roomCuts: [...openCuts, ...this._planSnapOpeningCuts(space, openCuts)]` + (буквально то же выражение, что было на `origin/dev` до `9f77e3e`, см. + `git show origin/dev:src/houseplan-card.ts` — метод `_planSnapOpeningCuts` + существовал там же). Новый `_planStructuralGeometrySnapshot` использует + `roomCuts: this._openCuts()` — ровно то, что `_planSnapGeometrySnapshot` + вычислял в промежуточном (r1) коде. Все потребители разделены корректно и + без остатка: `_wallGraphSources` (`:6900-6901`, единственный вызывающий + `_wallFaceGraph`/#185-путь) — на структурный; четыре презентационных сайта + (`:6213, 6236, 11771, 17598, 17688`) — на исходный. Grep по всему файлу + подтверждает отсутствие смешанных вызовов. + +## Находки + +### High + +Нет. High-1 из r1 исправлен корректно (см. «Как проверялось» — фикс, тест и +зелёный CI). + +### Medium (заведена отдельным issue) + +#### Medium-1 → #189 — presentation snap-overlay не режет ось независимой перегородки в месте её собственного проёма + +Второй компонент этого коммита восстанавливает presentation-cut для +**legacy room-wall** openings и для **composite** partition/room-wall +случая, но не для основного сценария #132 — проёма на независимой +перегородке, не совпадающей ни с какой стеной комнаты. + +**Почему это не тривиальная догадка, а воспроизведённый дефект.** Прочитано +и эмпирически проверено (headless Chromium, `demo/serve.mjs`, тот же харнесс, +что у смоков): пространство с одной перегородкой `{a:[0.25,0.5], b:[0.75,0.5]}` +без комнат и дверью `host:{kind:'partition', id, t:0.5}` даёт +`card._planSnapGeometrySnapshot().value.segments` **один** сегмент `partition` +от `[250,500]` до `[750,500]` — без разрыва в точке проёма (x=500). Причина +прослеживается по коду: + +- `_roomWallOpeningInputs()` (`src/houseplan-card.ts:7834-7855`) для + partition-hosted проёма возвращает вход в выборку **только** когда + `partitionOpeningHasCompositeRoomWall(...)` истинно — т.е. только для + composite-случая (решение по Q4); +- `_planSnapOpeningCuts()` (`:6141-6154`) строит cuts исключительно из этой + выборки — для обычного (не composite) partition-hosted проёма cut не + создаётся вообще; +- `buildPlanSnapGeometry()` (`src/plan-snap-overlay.ts:118-156`) в любом + случае передаёt `cuts: []` для `kind: 'partition'` безусловно (`roomCuts` + применяется только к `kind: 'room'`, `:123-131`) — так что даже если бы cut + был вычислен, для partition-источника он никуда не попал бы без изменения + этой функции. + +**Почему это противоречит контракту, а не просто пробел evidence.** +ТЗ §11 буквально: «Контракт применяется одинаково к legacy room-wall opening +и новому partition-hosted opening» — про presentation/snap boundary cut. +Этот же коммит переписал `docs/CANVAS.md` («Door, window, gate and +intentionally open-span intervals are cut from presentation axes», без +оговорки про host kind) и оба changelog («the editor's visual snap guide +keeps its physical gap across the opening» / «визуальная направляющая +привязки по-прежнему показывает физический разрыв в месте проёма») — +универсально, без оговорки о composite-случае. Заявленное в документации и +реализованное в коде расходятся именно для главного, а не краевого сценария +#132 (проём на независимой, не совпадающей с комнатой перегородке). + +**Почему Medium, не High.** Не портит сохранённые данные, не ломает +physical/light геометрию (та режется верно — `hostBodyHasFullDepthOpeningGap` +в `smoke_partition_openings.mjs` зелёный) и не ломает structural room-face +граф (#185 — `_planStructuralGeometrySnapshot` намеренно и корректно +игнорирует cuts для обоих host kind, что и требуется). Затрагивает только +inline snap-подсказку инструмента «Стены» в Plan-редакторе (admin-only +поверхность): пользователь может получить снап на точку, физически лежащую +внутри проёма, как если бы там была сплошная кладка — ровно то поведение, +которое контракт «opening gap remains a gap» (#173) должен исключать, только +для нового host kind. Не покрыто ни одним из 12 AC ТЗ буквально (все они +описывают geometry/light/HA/render — не snap-guide), поэтому не проваливает +формальный AC, но нарушает явный текст §11 и текст только что обновлённой +документации/changelog в этом же коммите. + +Заведено: #189 (bug, P2, S1-new), со ссылкой на #132 и точной репродукцией. + +### Low + +Нет новых Low в этом цикле. Три Low из SPEC-REVIEW-132-r1 были закрыты до +начала кода (запись автора «Начало реализации»); не пересматривались здесь, +диапазон `3fe0f8c` их не касается. + +## AC — статус после r2 + +AC1–AC4, AC6–AC8, AC10–AC12: без изменений относительно r1 (**пройдены**, +диапазон `3fe0f8c` их кода не касается — подтверждено чтением diff-состава). + +AC5 (#185 room closure): **пройден**, подтверждено заново для этого коммита +(`smoke_room_autoclose.mjs`: `openingKeepsStructuralAutoClose: true`; +`smoke_partition_openings.mjs`: `openingKeepsRoomFaceAxisContinuous: true` +через переименованный, но не изменённый по семантике метод). Разделение +снапшотов не меняет вход `_wallFaceGraph` — структурный снапшот вычисляется +идентично тому, что использовался в r1. + +AC9 (compatibility): **пройден** — это ровно то, что чинил High-1. Backend +export/import/duplicate одного space с partition-hosted openings теперь +сохраняет referential integrity `host.id`; подтверждено новым регрессионным +кейсом в `test_ha_import_export.py` и зелёным `backend` job на точном SHA +`3fe0f8c` в Linux CI (280 passed, 0 failed). + +Ни один из 12 AC не описывает presentation snap-overlay буквально (см. +Medium-1/#189) — находка не проваливает формальный AC, но является +нарушением §11 ТЗ вне списка AC1–AC12. + +## Что проверено и корректно + +- High-1 из r1 фактически исправлен: код читается корректно (порядок ремапа, + тот же `id_map`, что у остальных ссылок), новый regression-тест + целенаправленно бьёт по сценарию High-1 (фикстура с `host`), Linux CI + backend job зелёный на точном SHA с командой и результатом в логе. +- Presentation/structural разделение снапшотов реализовано чисто: ни одного + оставшегося смешанного вызова (`_wallGraphSources` — единственный + потребитель структурного, четыре презентационных сайта — исходного; + проверено grep по всему файлу). +- Golden CI зелёный полностью (все перечисленные сцены `passed`, включая + ранее неразобранную `plan-snap-line-gaps-dark`), лучше результата r1. +- Три Medium из r1 (#186, #187, #188) корректно остаются отдельными + открытыми issue, не включены и не спрятаны в этом фиксе — проверено + `gh issue view` по каждому: все три открыты, `S1-new`, ссылаются на #132. +- Трейлеры коммита `3fe0f8c`: `Issue: #132`, `User-Visible: yes`, оба + changelog (RU+EN) правлены в этом же коммите — проверено `git show --stat`. +- Bundle freshness: три копии (`dist/`, `custom_components/houseplan/frontend/`, + `demo/srv/assets/`) побайтово идентичны после чистой пересборки в этой + сессии; `git status --short` после сборки пуст. + +## Чего не проверял + +- Полный набор из 141 браузерного смока — не запускал, diff 16-файлового + фикса не касается большинства поверхностей (обоснование выбора трёх + целевых смоков — выше). CI job `smoke` на момент завершения этого документа + ещё выполнялся (`in_progress`) — полный прогон относится к предрелизному + гейту, не к гейту этого код-ревью, и не блокирует вердикт. +- `tests_backend/test_ha_import_export.py` не выполнялся мной локально (нет + `homeassistant` в этом окружении) — заменено проверкой факта и результата + прогона на Linux CI на точном SHA (см. «Как проверялось»), а не + предположением. +- `performance_smoke` — не запускал, diff не касается кеш-инвалидации/hot-path + (только backend id-ремап и разделение уже кешируемых снапшотов по тому же + шаблону, что был). +- Composite (coincident partition + room-wall) presentation-cut сценарий — не + проверял отдельно эмпирически в этом цикле; логика `_roomWallOpeningInputs` + не менялась в `3fe0f8c` и была прочитана r1 как корректная для этого случая + (см. CODE-REVIEW-132-r1, разбор AC2). Мой репродукшн для #189 намеренно взял + **не**-composite случай, чтобы изолировать дефект. +- Точную причину, почему исходный golden-diff `plan-snap-line-gaps-dark` + использовал именно room-wall, а не partition-сценарий (и поэтому не + вскрыл #189 в r1/r2 через golden) — не расследовал; вне golden-набора нет + сцены с независимой перегородкой и проёмом без совпадения с комнатой, + что и объясняет, почему CI не поймал #189. + +## Вердикт + +Зелёный · цикл r2/4 · High: 0 · Medium: 1 → #189 + +High-1 из r1 исправлен и подтверждён (код + новый тест + зелёный Linux CI +backend job на точном SHA). Остальные 11 AC не затронуты этим диапазоном и +остаются в состоянии r1. Новая Medium-находка (#189) — presentation +snap-overlay не режет ось независимой перегородки в месте собственного +проёма, вопреки §11 ТЗ и тексту этого же changelog — не проваливает ни один +из 12 сформулированных AC буквально, не портит данные, не затрагивает +structural/#185-путь и не расширяется на View/kiosk; заведена отдельным issue +и не блокирует переход, как и три предыдущих Medium из r1.