mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-05 06:08:59 +00:00
claude[bot]
@@ -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`.
|
||||
Reference in New Issue
Block a user