From a0c4c1cdd8081a48a38073e4cb078fc4412adb06 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Wed, 19 Aug 2026 13:00:05 +0000 Subject: [PATCH] docs: code review r1 for #186 Issue: #186 User-Visible: no --- docs/reviews/CODE-REVIEW-186-r1.md | 235 +++++++++++++++++++++++++++++ 1 file changed, 235 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-186-r1.md diff --git a/docs/reviews/CODE-REVIEW-186-r1.md b/docs/reviews/CODE-REVIEW-186-r1.md new file mode 100644 index 00000000..bec7db1b --- /dev/null +++ b/docs/reviews/CODE-REVIEW-186-r1.md @@ -0,0 +1,235 @@ +# CODE-REVIEW-186-r1 — Безопасный остаток стены у торцов партиционного проёма + +Issue: [#186](https://github.com/Matysh/houseplan-card/issues/186) +ТЗ: `docs/specs/186-partition-opening-jamb-margin.md` (зелёное ревью, +`docs/reviews/SPEC-REVIEW-186-r2.md`) +Диапазон: `origin/dev..HEAD` — единственный продуктовый коммит +`57ba75b` «fix: keep jamb margin on partition openings» +(`Issue: #186`, `User-Visible: yes`, оба changelog в том же коммите). +Ветка: `issue/186-partition-jamb-margin`. + +## Скоуп + +Введён физический jamb safety margin (половина фактической толщины +независимой стены) у обоих торцов hosted-проёма (`door`/`window`/`gate`/ +`passage`): разделены compat/read (`resolvePartitionOpeningCompat`, margin=0) +и strict/write (`resolvePartitionOpeningStrict`) резолверы, placement и drag +клэмпятся к новой границе, dialog/rebind объясняют отказ локализованным +сообщением, backend `config/set`/`optimize` применяют тот же контракт как +semantic delta validation, а structural schema и full backup/restore остаются +на прежней zero-margin границе, чтобы не терять уже сохранённые near-end +проёмы. Соответствует J4/J6 (`docs/SCOPE.md`) и не расширяет скоуп ТЗ. + +## Как проверялось + +| Гейт | Результат | +|---|---| +| `npx tsc --noEmit` | green | +| `npm test` | green, 906/906 | +| `npm run build` + сверка трёх копий бандла (`dist`, `custom_components/.../frontend`, `demo/srv/assets`) | green, побайтово идентичны | +| `node demo/smoke_partition_openings.mjs` (назван в AC2/AC3, поверхность прямо тронута) | green, все поля результата `true`, включая 5 новых (`placementClampsToHalfThicknessJamb`, `directDragStopsAtSameJambBoundary`, `dialogShowsJambSpecificGuidance`, `invalidLengthWritesNeitherConfigNorHistory`, `tooShortPartitionHasNoPreviewAndExplainsWhy`) | +| `python -m pytest tests_backend/test_validation.py -q` (только этот файл тронут напрямую) | green, 132/132 (совпадает с хендоффом автора) | +| `python -m pytest tests_backend -q` (полный офлайн-прогон, без `homeassistant`) | green, 165 passed; `test_ha_import_export.py`/`test_ha_websocket.py` молча пропущены `conftest.py` — HA не установлена в среде ревью, это ожидаемо (`AGENTS.md`: канонический прогон — Linux CI) | +| `npm run golden:verify` (полный набор, Linux) | green, все сценарии `passed`, включая `opening-placement-door-thick-wall-dark`, `opening-placement-passage-thick-wall-{dark,light}`, `openings-thick-wall-dark`, `openings-filled-tunnel-dark`, `openings-hidden-view-dark` — прогнан сверх минимума, т.к. diff меняет clamp/placement геометрию, а не только текст | +| `node demo/smoke_opening_measure.mjs` (побочная проверка того же placement-пути) | green (в этой среде извечная pixel-precision особенность из `AGENTS.md` не воспроизвелась) | + +### Не проверялось (и почему) + +- `tests_backend/test_ha_import_export.py`, `tests_backend/test_ha_websocket.py` — + новые тесты в этих файлах **не исполнены**: в среде ревью нет `homeassistant` + и нет `.venv-backend` (он есть только на облачных агентах по `AGENTS.md`). + Разобраны построчным чтением (см. AC4/AC6 ниже) — «проверено чтением, не + исполнением». +- Полный browser smoke suite (127 файлов) — не прогонялся; diff не задевает + весь фронтенд, только `partition-openings`/`opening-placement`/opening + dialog. Прогнаны целевой smoke плюс побочный `smoke_opening_measure`. +- `performance_smoke` — не прогонялся; AC не называет perf-эффект, а ТЗ §13 + обосновывает `O(1)` без новых проходов геометрии; изменение ограничено + сравнением чисел на уже существующих объектах. +- Ручного тестирования в UI не было (по процессу его нет в цикле). + +## Проверка AC + +### AC1 — единая физическая граница + +**Доказано автотестом, тест умеет падать.** `test/partition-openings.test.mjs` +(«strict jamb policy follows thickness, scale and host direction») перебирает +`cm ∈ {1,15,100}`, `(cellCm,gridPitch) ∈ {(5,5),(2.5,8)}` и оба направления +хоста, вычисляет `exactT` **независимо** от кода через ту же явную формулу и +проверяет: точная граница — resolved, `exactT - 1e-5` — `does-not-fit-jamb`. +Backend-зеркало — `tests_backend/test_validation.py::test_partition_opening_jamb_margin_exact_boundary_and_epsilon`, +параметризован по тем же `cm`/`cell_cm`, обоим направлениям и всем четырём +типам проёма; исполнен зелёным (132/132 включает эти кейсы). Независимая +проверка чтением: `partitionOpeningJambMargin` (`src/partition-openings.ts:36-40`) +и backend `margin = cm/2/cell_cm/240` (`validation.py:141-143`) — та же формула, +что зафиксирована в ТЗ §4, `240` вынесена в именованную константу +`NORMALIZED_CANVAS_CELLS` с комментарием о синхронизации с `GRID_STEP_N`/`NORM_W`. +Тест на равенство границы **умеет падать** — если margin/эпсилон перепутать +знаком или множителем, `exactT`/`exactT-1e-5` разойдутся с реальным резолвером +(значения жёстко вычислены в тесте, а не взяты из его же вывода). + +### AC2 — placement и drag не создают плохую geometry + +**Доказано автотестом + smoke, тест умеет падать.** +`resolveOpeningPlacementResult` (`src/opening-placement.ts:258-296`) требует +`renderedLength + 2·physicalHalfWidth <= hostLength` для eligibility и +клэмпит `along` в `[half, length-half]`, где `half = renderedLength/2 + jambMargin` +(строки 320-338) — совпадает с ТЗ §7.2 дословно. `test/opening-placement.test.mjs` +проверяет конкретные числа (`x=25`/`x=75`, `t=0.25`/`t=0.75`, +`labels[i].distance=10`) для partition-хоста и **отдельно** нулевой jamb для +room-wall (`distance=0`) — тест на регрессию §7.2 «room-wall placement +сохраняет прежнюю границу». Смок `demo/smoke_partition_openings.mjs` +подтверждает end-to-end через реальный DOM/pointer events: клик у торца +клэмпится (`placementClampsToHalfThicknessJamb`), drag существующего проёма к +тому же торцу останавливается на той же границе +(`directDragStopsAtSameJambBoundary`), слишком короткая partition не даёт +preview и объясняет причину через toast (`tooShortPartitionHasNoPreviewAndExplainsWhy`). +Прочитан код клика/drag (`src/houseplan-card.ts:11298-11321` и `:11482-11500`) +— оба пути используют один и тот же resolved-кандидат (`jambMargin = +partitionOpeningJambMargin(...)`, `shoulder = half + jamb`), других мест +клэмпа для partition-хоста не найдено. + +### AC3 — dialog и rebind объясняют отказ + +**Доказано smoke + разбором кода.** `dialogShowsJambSpecificGuidance` и +`invalidLengthWritesNeitherConfigNorHistory` в `smoke_partition_openings.mjs` +воспроизводят реальный сценарий: `_editOpening` → увеличение длины до 600 см +→ banner с RU/EN текстом margin → `_saveOpening()` не меняет ни `space.openings` +(сравнение `JSON.stringify` до/после), ни счётчик записей конфига (`writes`), +ни `_geometryHistory`. Тест умеет падать: до правки banner показывал бы +`opening.host_partition` вместо margin-текста, а `_saveOpening()` записал бы +конфиг — оба факта проверяются прямым сравнением, а не полагаются на +побочный эффект другого поля. Разбор кода (`src/houseplan-card.ts:17641-17694`) +подтверждает, что dialog различает `orphan` (missing host) и `jambInvalid` +(does-not-fit-jamb) отдельными флагами и не путает их тексты; `lengthTouched` +(`:1342`, `:11635-11636`, `:17650-17655`) корректно не опто́м вводит длину +округлённого legacy-значения в strict-проверку при binding-only правке (type/ +contact/lock/invert/flip) — независимая проверка по коду совпадает с ТЗ §7.3 и +принятым предположением №2 (§16). Rebind: `orphanRebindReplacesHostWithoutDuplicate` +в том же smoke подтверждает успешный rebind на валидный host (существующий +сценарий #132, не задет регрессией). + +### AC4 — legacy compatibility + +**Доказано автотестом (frontend) + разбором кода и backend-тестами (не +исполнены).** Frontend: «strict writes reserve half the real wall depth while +compat reads remain visible» (`test/partition-openings.test.mjs`) явно строит +near-end проём (`t=0.224`, внутри jamb) и проверяет, что `resolvePartitionOpening` +(compat, margin=0) всё ещё резолвит его для всех четырёх типов, тогда как +`resolvePartitionOpeningStrict` возвращает `does-not-fit-jamb` — то есть тест +умеет различать «остался видимым» и «отклонён при прямой правке» и упал бы, +если бы кто-то заменил compat-путь на strict в рендере. `_physicalUp` (rigid +translation, `:7472-7480`) и все read-пути (`space-render.ts:214`, +`houseplan-card.ts:7940,7973,11324,11667`) используют `resolvePartitionOpeningCompat` +— построчно проверено, что бывший «голый» вызов `resolvePartitionOpening` с +неявным `jambMargin=0` (сама находка issue) нигде не остался: `grep` по всем +вызовам показывает только `Compat`/`Strict`, внутренний `resolvePartitionOpening` +вызывается лишь изнутри этих двух обёрток. + +Backend delta-валидатор (`custom_components/houseplan/validation.py:110-152`) +включает strict-проверку только если `host.id`/`host.t`/`opening.length`/ +`partition.cm`/`span` изменились относительно `previous` — **прочитано и +независимо пересчитано**: rigid-перенос (одинаковый `span`, одинаковый `cm`, +одинаковые `host.id/t/length`) не триггерит strict, посторонняя правка (смена +`title`) тоже. `tests_backend/test_validation.py::test_partition_opening_jamb_delta_preserves_legacy_and_checks_direct_geometry` +(исполнен, зелёный) покрывает четыре ветки: unrelated-edit, type-only, rigid +translation — allow; и четыре прямых геометрических изменения (`t`, `length`, +`cm`, `span` через новое `b`) — reject. Тест умеет падать: если `strict` +неверно посчитать как `False` для любой из четырёх мутаций, `pytest.raises` +не сработает. + +Full import/restore: **прочитано, не исполнено** — +`custom_components/houseplan/import_export.py:1324-1334` (`prepare_apply`, +`kind == "full"`) не проходит через `build_space_merge`/ +`validate_partition_opening_hosts` вообще, только через структурную +`CONFIG_SCHEMA` на выходе (та же неизменная `does-not-fit` проверка, +`validation.py:906-925`, unrelated diff). Значит full restore физически не +может получить `PartitionOpeningJambMarginError` — соответствует decision #4 +и §8 ТЗ. Новый тест `test_full_preview_preserves_legacy_near_end_partition_opening` +(`tests_backend/test_ha_import_export.py`, не исполнен: нет HA) параметризован +по `current_kind ∈ {"empty","other"}`, что покрывает decision #4 дословно +(«на пустой и на другой инсталляции»); по чтению кода `create_preview` +(`import_export.py:1141-1181`) не зависит от `current_config` для ветки +`kind != "space"`, так что ожидание «зелёный на обоих current_kind» логически +устойчиво, но сам прогон не подтверждён исполнением — фиксирую как честный +пробел, не блокирующий (см. «чего не проверял»). + +### AC5 — overlap и остальные host не меняются + +**Проверено чтением, не исполнением, плюс существующий регресс-набор.** +`hostedOpeningIntervalsOverlap` не встречается в диффе (`git diff` пуст по +этой функции в обоих языках); overlap-проверка backend +(`validation.py:920-924`) и структурная fit-проверка (`:917-919`) — те же +строки, что до PR, jamb margin в них не подмешан. Существующие regression-тесты +#132/#157/#193 остаются в `npm test`/`pytest` без изменений и зелёные. + +### AC6 — server parity и error code + +**Доказано backend/websocket-тестами.** `config/set` и `plan/optimize` +(`websocket_api.py:1258-1263`, `:1373-1378`) оборачивают один и тот же вызов +`validate_partition_opening_hosts` и один except-список с новым +`PartitionOpeningJambMarginError`; `tests_backend/test_ha_websocket.py::test_config_writers_reject_partition_opening_without_jamb_atomically` +параметризован по обоим эндпоинтам и проверяет атомарность (`config/get` и +`layout/get` после отказа возвращают исходные `rev`/данные) — **не исполнен** +в этой среде (нет HA), разобран по коду: оба хендлера действительно вызывают +валидатор до записи (`await`/`store.async_save` после except-блока в обоих +местах, построчно сверено) и `send_error(msg["id"], err.code, str(err))` +формирует `error.code == "invalid_partition_opening_jamb_margin"` и +сообщение, содержащее `space=…; opening=…; margin=…; margin_cm=…` +(`validation.py:59-68`) — совпадает с тестовыми ассертами дословно. Structural +schema (`SPACE_SCHEMA`, unchanged `does-not-fit`) не участвует в этой цепочке +и не может «сломать» round-trip неизменённого конфига — совпадает с §8. + +## Что проверено и корректно (сверх AC) + +- **Формула единиц исполнена согласованно.** `wallCmToUnits(cm, cellCm, + gridPitch) = clampWallCm(cm)/cellCm*gridPitch` — линейна по `gridPitch`, + поэтому обратное преобразование `(margin/gridPitch)*cellCm` в + `houseplan-card.ts:12816-12819` алгебраически сводится ровно к `cm/2`; + проверено пересчётом руками, не только чтением. +- **Единственное расхождение в способе получения `distance` для диалогового + banner** (`src/houseplan-card.ts:17669-17671`, `jambDistance = + formatLength(hostPartition.cm / 2, …)` — напрямую через `cm`, а не через + `partitionOpeningJambMargin` + обратная конвертация, как везде в других + местах) **не является дефектом**: математически идентично при валидном + `partition.cm`; расхождение возможно только если `partition.cm` вне + `[WALL_MIN_CM, WALL_MAX_CM]` до `clampWallCm`, а ТЗ §16.4 прямо говорит, что + malformed `cm` вне scope #186 и сохраняет действующий schema/fail-dark + contract — сохранённый `partition.cm` уже проходит schema. Отмечаю как + **Low, снята без правки**: чисто стилистическая дупликация формулы, не + влияет ни на один AC и не воспроизводима в валидном состоянии данных. +- **Wall-thickness edit partition.cm вне opening dialog** (`_physicalDialog`, + `houseplan-card.ts:7257-7259`) не проходит через + `partitionOpeningNeedsStrictValidation` на фронтенде — и не должно: ТЗ §7.4 + прямо поручает этот случай backend-strict-границе («Изменение длины host или + его `cm`… должно пройти strict backend boundary»), и backend-валидатор + действительно триггерит `strict` при изменении `partition.cm`/`span` + (проверено выше, AC4/AC6). Отказ backend будет показан пользователю через + тот же generic error-mapping (`_mapConfigError`-подобный путь, + `houseplan-card.ts:12812-12820`), это тот же механизм, что уже работает для + `invalid_partition_opening_host`/overlap с #132 — не новый риск. +- **i18n** (`opening.partition_jamb_margin`) добавлен синхронно en/ru и + дословно совпадает с формулировкой ТЗ §7.3. +- **Документация**: `docs/CHANGELOG.md`/`.ru.md`, `docs/USER-GUIDE.md`/`.ru.md`, + `docs/CONFIG-COMPATIBILITY.md`, `docs/ARCHITECTURE.md`, `docs/TESTING.md` + обновлены в том же коммите, терминология («независимая стена», «торец», + «откос»/jamb) соответствует `docs/USER-GUIDE.ru.md`, не изобретена заново. +- **Трейлеры и changelog-требование**: единственный продуктовый коммит несёт + `Issue: #186` и `User-Visible: yes`, оба changelog правлены в нём же — + соответствует PROCESS.md §2.6/§10.2. + +## Находки + +Нет High. Нет Medium — ни в скоупе, ни вне скоупа. Один Low, снят без правки +(см. выше, «Единственное расхождение…») — чисто стилистический, не +воспроизводится при валидных данных, не влияет ни на один AC. + +## Вердикт + +Зелёный. Все шесть AC либо доказаны исполненным автотестом с проверенной +способностью упасть, либо разобраны построчным чтением с явной пометкой +«проверено чтением, не исполнением» там, где локальный HA-harness недоступен. +Формула margin идентична на frontend и backend, все исходные call sites из +диагноза issue закрыты явным compat/strict выбором, legacy near-end проёмы +и full backup/restore подтверждённо не регрессируют, golden-набор (полный, +Linux) зелёный без нового baseline.