From b268dd26838842323582648defbb3ea5f2203e60 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Fri, 14 Aug 2026 12:11:07 +0000 Subject: [PATCH] docs: review document for #141 Issue: #141 User-Visible: no --- docs/reviews/CODE-REVIEW-141-r2.md | 218 +++++++++++++++++++++++++++++ 1 file changed, 218 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-141-r2.md diff --git a/docs/reviews/CODE-REVIEW-141-r2.md b/docs/reviews/CODE-REVIEW-141-r2.md new file mode 100644 index 00000000..02bd582f --- /dev/null +++ b/docs/reviews/CODE-REVIEW-141-r2.md @@ -0,0 +1,218 @@ +# Code review — issue #141, cycle r2 + +Вердикт: **зелёный** · цикл r2/4 · High: 0 · Medium: 0 + +Ветка: `issue/141-wall-joints` · head-коммит +[`f9476c3`](https://github.com/Matysh/houseplan-card/commit/f9476c3f78c242b25344fa190c801c27b839c71c) +(`fix: preserve single wall previews`) поверх implementation-коммита +[`3e33f4a`](https://github.com/Matysh/houseplan-card/commit/3e33f4a5845a29694473697bea916bb3e2490ac2), +проверенного в [`CODE-REVIEW-141-r1.md`](CODE-REVIEW-141-r1.md) (красный, +High: 1). ТЗ: [`docs/specs/141-wall-junctions.md`](../specs/141-wall-junctions.md) +(reviewed `2858175`, зелёный `SPEC-REVIEW-141-r1.md`). + +## Скоуп проверки + +r1 нашёл один блокирующий High и остановился на нём (правило «дороже искать +второй дефект, чем дешевле починить первый»). r2 проверяет: (а) правку этого +конкретного High по существу, а не только «тест прошёл»; (б) что фикс не +сломал ничего из уже подтверждённого в r1 по остальным 12 пунктам; (в) что +изменённый диапазон (`git diff origin/dev...HEAD`, теперь 26 файлов) не +содержит новых незамеченных изменений сверх точечного коммита `f9476c3`. + +`git show f9476c3 --stat` — 7 файлов: `src/wall-thickness.ts` (+6/-1), +`test/wall-thickness.test.mjs` (+2), оба changelog (+3/-1 каждый), три копии +бандла. Это ровно тот минимальный набор, который требуется для точечного +фикса: не расширяет диф `3e33f4a`, не трогает `physical-geometry.ts`, +`space-render.ts`, `houseplan-card.ts` — весь остальной код, уже проверенный в +r1, не менялся между r1 и r2. + +Трейлеры `f9476c3`: `Issue: #141` · `User-Visible: yes`; оба changelog правят +ту же строку записи `3e33f4a` (не добавляют новую) в этом же коммите — +требование выполнено. Финальный коммит диапазона `44ba55d` — только +`docs/reviews/CODE-REVIEW-141-r1.md`, класс C, не влияет на проверяемое +поведение. + +## Как проверялось + +### Разбор фикса по коду (не только «тест зелёный») + +`src/wall-thickness.ts:622-636` (`unionSimpleBodies`): + +```ts +geom = geom ? union(geom, piece) : [piece]; // было: ... : piece; +``` + +`closedRing()` (`src/wall-thickness.ts:1355-1359`) возвращает `[ring]` — +одно кольцо, то есть значение типа `Polygon` (`Ring[]`) в терминах +`polyclip-ts`. `union(a, b)` всегда возвращает `MultiPolygon` (`Polygon[]`). +До фикса единственное тело присваивалось в `geom` как голый `piece` +(`Polygon`), а не как `MultiPolygon` — ровно расхождение форм, которое r1 +нашёл: `polyclipToPathD()` итерирует `for (const poly of geom) for (const +ring of poly)`, и на голом `Polygon` внешний цикл видел не полигоны, а сами +точки кольца. После фикса `[piece]` — это `Polygon[]` с одним элементом, то +есть корректный `MultiPolygon` формы, которую в норме возвращает `union(...)`. +Дальнейшие итерации (`geom ? union(geom, piece) : ...`) не менялись и уже были +верны — правка точечная и минимальна, не переписывает остальную функцию. + +Проверено также, что это не механическая правка вслепую: `piece` не проходит +через `union()` на этом шаге (для одного простого, не самопересекающегося +quad/patch это не нужно — обёртка формы эквивалентна нормализации через +`union()` одного полигона), что соответствует уже принятой в кодовой базе +семантике `unionBodies()` (`src/physical-geometry.ts:148-155`, где единственное +тело тоже уходит через `union(polygons[0])`, гарантированно возвращающий +`MultiPolygon`) — именно то соответствие, которое r1 требовал восстановить. + +Проверено прямым вызовом собранного `test-build`, что регресс правда снят: + +``` +node -e " +const { drawWallPreviewD } = await import('./test-build/wall-thickness.js'); +console.log(drawWallPreviewD([[0,0],[100,0]], 8, false)); +" +``` +— возвращает непустой путь `M ...` (до фикса возвращал `''`, как задокументировано +в `CODE-REVIEW-141-r1.md`). + +Новый unit-тест (`test/wall-thickness.test.mjs:972-973`) закрывает именно +пропущенный в r1 случай — ровно один сегмент без соединений — и по формату +идентичен уже существующим утверждениям в том же тесте (не декоративный +`assert.ok(true)`; проверяет `includes('M')`, то есть непустой путь). + +### Дешёвые гейты (всегда) + +- `npx tsc --noEmit` → **зелёный**, без вывода. +- `npm test` → **793/793 green** (Linux; Windows-only `process-gate.test.mjs` + здесь не воспроизводится — ожидаемо, как и в r1). +- `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` после билда пуст (закоммиченные + копии соответствуют исходникам, дрифта нет). + +### Гейты по необходимости + +- `node demo/smoke_wall_junctions.mjs` (назван в ТЗ §13.2, покрывает + AC1/AC2/AC4/AC6/AC7/AC8/AC9) — **12/12 green**, включая + `lineTargetGetsLocalJoinPatch: true` — именно ту проверку, которая упала в + r1. +- `node demo/smoke_wall_thickness.mjs` — прогнан дополнительно, потому что + автор в комментарии к `f9476c3` сообщил о втором падении того же коммита + `3e33f4a` (пропавший hover одного участка стены) и точечно исправил его тем + же изменением `unionSimpleBodies`. Регресс-риск общий с основным фиксом, а + тест не назван в ТЗ #141 явно — стоило перепроверить отдельно. **28/28 + green**, включая `hover: true`. +- `node demo/smoke_glow.mjs`, `node demo/smoke_sun.mjs`, + `node demo/smoke_isometric_contract.mjs` — прогнаны точечно, потому что + `unionSimpleBodies` используется в `drawWallPreviewD`, который делит код с + путём, питающим `wallBodiesGeometry` (общая joined-геометрия для Glow/sun/iso + по AC7/AC8); фикс расположен в общем helper'е, а не в изолированном месте. + Все три **green** (27/27, 27/27, 16/16) — фикс не задел эти поверхности за + пределами уже проверенного. +- `npm run golden:verify` (полный набор 67 сценариев) — **не прогонялся**, по + той же причине, что и в r1: инструмент запрещает частичный прогон, полный + прогон — предрелизный Chromium-гейт (`AGENTS.md`: «smoke, golden и + performance_smoke... belong to the pre-release run»), а диф r2 — шесть + добавленных строк в одной функции плюс один unit-тест. Риск того, что именно + эта точечная правка сдвинула какой-то из 67 golden-baseline, не + подтверждается ни диффом (никакая геометрия узла/mitre/bevel не менялась, + правка только про форму возвращаемого значения для случая с одним телом, + который в закрытых/многосегментных golden-сценариях не возникает), ни + прогнанными smoke. +- `python -m pytest tests_backend` — не прогонялся: Python не тронут ни в + `3e33f4a`, ни в `f9476c3` (AC12). +- Performance-профили — не прогонялись: правка не меняет алгоритмическую + форму (не добавляет проходов, не трогает кеш/fingerprint), только форму + возвращаемого значения; AC11 в r1 подтверждён чтением кода и не затронут + этим коммитом. +- Полный browser smoke-suite (127 файлов) — не прогонялся целиком; прогнаны + именной smoke задачи плюс три смежные по трогаемому общему коду поверхности + (glow/sun/iso) — расширять дальше нет диффового повода. +- Полный локальный визуальный просмотр в браузере (не headless) — не + выполнялся; как и в r1, вывод строится на смоках/юнитах и прямом + воспроизведении на `test-build`. + +## Находки + +Находок уровня High и Medium нет. + +**Low, унаследованный из r1, не переоценивается заново:** экспортированная +`draftBodies()` (`src/physical-geometry.ts:71`) остаётся мёртвым кодом — +не изменилась между r1 и r2, автор явно отметил в комментарии к `f9476c3`, +что не стал удалять публично экспортируемый helper в рамках точечного +review-фикса. Решение разумно: `f9476c3` — узкий фикс с минимальным дифом +специально для быстрой повторной проверки; удаление отдельного мёртвого +экспорта в этом же коммите увеличило бы диф без необходимости. Остаётся Low, +не блокирует, правится по усмотрению автора в отдельной задаче или следующей +правке этого файла. + +**Побочное наблюдение вне скоупа #141 (не находка, не для этой задачи):** тот +же паттерн формы (`body = body ? union(body, piece) : piece;` без обёртки в +`[piece]`) присутствует в непотронутой этим диффом `exteriorEnvelopeGeometry()` +(`src/wall-thickness.ts:~1690-1745`, уже существовала на `origin/dev` до +#141). Не проверялось, воспроизводим ли там аналогичный дефект — код не входит +в диапазон `git diff origin/dev...HEAD`, никакая строка там не менялась ни в +`3e33f4a`, ни в `f9476c3`, и AC #141 не покрывают эту функцию. Упоминаю только +как наблюдение для владельца/следующего аналитика, не как Medium/High этого +ревью — заводить отдельный issue на непроверенное предположение о коде, не +относящемся к диффу, было бы самому выдавать догадку за факт. + +## Что проверено и корректно + +- **Исправление High из r1** — `unionSimpleBodies` теперь возвращает + корректный `MultiPolygon` и для одного, и для нескольких тел; подтверждено + чтением кода, прямым вызовом на `test-build` и зелёным + `smoke_wall_junctions.mjs` (`lineTargetGetsLocalJoinPatch: true`, + `rubberBandAndCommittedPreviewMatch: true`). +- **Побочный регресс, о котором сообщил автор** (`smoke_wall_thickness`, + пропавший hover одного участка) — исправлен тем же изменением; подтверждено + отдельным прогоном smoke (`hover: true`, 28/28). +- **Все 12 пунктов, подтверждённых в r1 по коду и smoke** (единая joined- + геометрия Plan/View/static/iso, clean-floor/Glow/sun через `physical` как + `extraBodies`, identity raw-тел, отсутствие записи конфига при + preview/hover, схема/бэкенд/i18n не тронуты, документация и бандлы в одном + коммите) — код между r1 и r2 в этих местах не менялся; повторно + подтверждено тем же `smoke_wall_junctions.mjs` (12/12) и точечными + `smoke_glow`/`smoke_sun`/`smoke_isometric_contract` (все green), чтобы + убедиться, что общий helper (`unionSimpleBodies`) не задел эти поверхности + при исправлении. +- **Регрессионный unit на ровно один сегмент** — новый ассерт в + `test/wall-thickness.test.mjs` содержателен (проверяет непустой путь для + двухточечного вызова), не тавтологичен, действительно ловит регресс: + временный откат правки к `: piece` (без `[...]`) заставляет этот ассерт + упасть (проверено локально откатом одной строки и повторным `npm test`). +- **Трейлеры и changelog `f9476c3`** — `Issue: #141` / `User-Visible: yes`, + правка той же строки в обоих changelog в этом же коммите; терминология + («hover толщины одиночного сегмента») согласуется с инструментом «Толщина» + из `docs/USER-GUIDE.ru.md` (:306, :346), не изобретает новый термин. +- **Три копии бандла** — идентичны друг другу и свежей локальной сборке; + `git status` после билда чист. + +## Чего не проверял + +- **`npm run golden:verify` (полный набор)** — не прогонялся; см. обоснование + в разделе «Как проверялось». Новые golden-сценарии `wall-junctions-*` / + `isometric-wall-junctions-dark` из r1 так и не просмотрены визуально ни в + r1, ни здесь — это остаётся открытым пунктом предрелизного гейта, а не + code-review, но фиксирую явно, чтобы решение не потерялось. +- **Полный browser smoke-suite (127 файлов)** — не прогонялся; прогнаны + целевой + три смежные по общему коду. +- **`python -m pytest tests_backend`** — не прогонялся, Python не тронут. +- **Performance smoke / Full Performance** — не прогонялись; диф не меняет + алгоритмическую форму горячего пути. +- **Ручное визуальное сравнение в браузере (не headless)** — не выполнялось. +- **Drag/Undo/Redo отдельных partitions после join** (часть AC9) — как и в + r1, отдельный интерактивный проход не делался; smoke проверяет только + сохранение количества/id raw-тел. +- **Побочное наблюдение про `exteriorEnvelopeGeometry()`** (см. «Находки») — + не исполнялось и не воспроизводилось; это код вне диапазона диффа #141. + +## Итог + +High: 0. Medium: 0. Low: 1 (унаследованный `draftBodies()`, не блокирует). + +Вердикт зелёный: High из r1 исправлен по существу (не только тест переписан — +разобрана причина, форма данных проверена по контракту `polyclip-ts`, +регресс-тест содержателен и способен падать), сопутствующий регресс +(`smoke_wall_thickness`) закрыт тем же изменением и отдельно перепроверен, +диф между r1 и r2 минимален и не выходит за рамки точечного фикса. Задача +может переходить в `S8-merged`.