mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
@@ -0,0 +1,201 @@
|
||||
# Code review #172 — r2
|
||||
|
||||
- **Issue:** https://github.com/Matysh/houseplan-card/issues/172
|
||||
- **Spec:** `docs/specs/172-zero-divider-taper.md`, зелёное `SPEC-REVIEW-172-r1.md`
|
||||
- **Reviewed branch:** `issue/172-zero-divider-taper` (HEAD detached at
|
||||
`origin/issue/172-zero-divider-taper`)
|
||||
- **Reviewed range:** `origin/dev..HEAD` = `93f86f8` (spec) → `56834c5` (spec
|
||||
review doc) → `c2112db` (fix, **User-Visible: yes**) → `eeb9c34` (code review
|
||||
r1 doc)
|
||||
- **Base:** `origin/dev` at `4d71f57` (уже включает #150 «preserve wall
|
||||
thickness transitions»)
|
||||
- **Reviewer:** Claude, независимая сессия без контекста реализации
|
||||
- **Причина цикла r2:** r1 был зелёным (`High: 0 · Medium: 0`), но слияние в
|
||||
`dev` конфликтовало; PROCESS.md §2.6/§10.4 требует повторного код-ревью
|
||||
после ребейза на ушедший вперёд `dev`, потому что это другой код. Ветка
|
||||
перебазирована автором на `origin/dev` `4d71f57` (включает #150), конфликт
|
||||
разрешён, коммит реализации переименован в `c2112db`. Цикл считается по
|
||||
этапу (§10.4): вердикт по ТЗ не расходует бюджет код-ревью, это первая
|
||||
расходующая бюджет код-ревью правка → `r2/4`.
|
||||
|
||||
## Вердикт
|
||||
|
||||
**Зелёный · цикл r2/4 · High: 0 · Medium: 0.**
|
||||
|
||||
Продуктовый диф после ребейза **содержательно идентичен** дифу, уже
|
||||
проверенному в `CODE-REVIEW-172-r1.md`: те же 25 строк в
|
||||
`src/wall-thickness.ts` (симметричная ветка «локальный cap» в
|
||||
`insetContour()`/`outsetContour()`), тот же набор тестов, тот же smoke, та же
|
||||
golden-сцена (версия матрицы 25), та же документация и оба changelog в одном
|
||||
коммите. Единственное отличие — коммит стал `c2112db` вместо `dfd56e8` (другой
|
||||
SHA после ребейза на `dev`, содержащий #150) и второй код-ревью документ
|
||||
(`eeb9c34`) добавлен как отдельный класс-C коммит.
|
||||
|
||||
Я не унаследовал вывод r1 не глядя: пересобрал бандл, независимо повторил
|
||||
дисциплину «тест умеет падать» (временно откатил обе новые ветки в коде и
|
||||
получил 2 красных unit-теста и 2 красных поля в named-smoke), прогнал полный
|
||||
юнит-регресс и четыре смежных/зависимых browser-smoke, включая smoke #150
|
||||
(`smoke_wall_thickness_transition.mjs`), которого не было в списке r1, потому
|
||||
что на момент r1 #150 не был частью проверяемого дерева — теперь он есть, и
|
||||
обе правки одной и той же общей geometry-функции сосуществуют без конфликта
|
||||
поведения.
|
||||
|
||||
## Скоуп
|
||||
|
||||
Единственный продуктовый файл — `src/wall-thickness.ts`: `insetContour()` и
|
||||
`outsetContour()` получили симметричную ветку `if ((oA > 0) !== (oB > 0))`,
|
||||
которая перехватывает переход «положительный offset ↔ ровно нулевой offset» до
|
||||
общей mitre/bevel- и collinear-логики, помещённую **перед** веткой
|
||||
`collinearJoint()`. Для точного коллинеарного перехода обе ветки вычисляют
|
||||
одну и ту же точку (`nA === nB` при совпадающем направлении), поэтому
|
||||
перестановка порядка проверок не меняет поведение AC3 (существующая точная
|
||||
ступень).
|
||||
|
||||
Сопутствующие изменения (не поменялись с r1): `test/wall-thickness.test.mjs`
|
||||
(два новых теста), `demo/smoke_zero_divider_taper.mjs` (новый),
|
||||
`demo/golden/{harness,matrix}.mjs` + `test/golden-matrix.test.mjs` (новая
|
||||
сцена `split-zero-divider-taper-dark`, `GOLDEN_MATRIX_VERSION` 24→25, baseline
|
||||
сознательно не принят), `docs/WALL-THICKNESS.md` §3 (контракт cap
|
||||
задокументирован), `docs/CHANGELOG.md`/`docs/CHANGELOG.ru.md`, три синхронные
|
||||
копии бандла, `docs/specs/README.md`.
|
||||
|
||||
Ровно один продуктовый коммит `c2112db`, трейлеры `Issue: #172` /
|
||||
`User-Visible: yes` на месте, оба changelog в том же коммите (проверено
|
||||
`git show --stat c2112db`). Ветка называется по правилу, `process-gate.mjs`
|
||||
проходит на всём диапазоне (4 коммита, 0 предупреждений).
|
||||
|
||||
## Как проверялось
|
||||
|
||||
| Гейт | Результат |
|
||||
|---|---|
|
||||
| `npx tsc --noEmit` | pass, без вывода |
|
||||
| `npm test` | **833/833 pass** (было 830/830 в r1 — разница объясняется тремя тестами #150, которые вошли в базовый `dev` при ребейзе; сами тесты #172 те же два) |
|
||||
| `npm run build` + сверка трёх копий бандла | pass; `sha256sum dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js demo/srv/assets/houseplan-card.js` → один и тот же хеш `21f8ffc…3233e`; `git status --short` после копирования — пусто, бандл в дереве уже актуален |
|
||||
| Откат обеих новых веток в `insetContour`/`outsetContour`, повторный `npm test` | **831/833**, красные ровно `variable-offset contours keep a local cap at angled positive-to-zero joins` и `near-collinear zero-depth Split divider never grows a masonry taper` — дисциплина «тест умеет падать» подтверждена мной лично на пересобранном дереве, а не переиспользована из r1 |
|
||||
| Пересборка **без** фикса + `node demo/smoke_zero_divider_taper.mjs` | `planHasNoTaper: false`, `lightHasNoTaper: false`, `FAILED (2)` — smoke тоже подтверждённо умеет падать |
|
||||
| Восстановление фикса, пересборка, синхронизация трёх копий бандла | все три sha256 совпадают между собой и с закоммиченным деревом |
|
||||
| `node demo/smoke_zero_divider_taper.mjs` (AC6/AC7) | pass, все 13 полей `true` |
|
||||
| `node demo/smoke_split_corner_wall.mjs` (смежная поверхность, AC3/AC9) | pass |
|
||||
| `node demo/smoke_wall_thickness.mjs` (смежная поверхность) | pass |
|
||||
| `node demo/smoke_wall_junctions.mjs` (смежная поверхность, T/L-стыки) | pass |
|
||||
| `node demo/smoke_wall_thickness_transition.mjs` (#150 — та же общая функция, слита при ребейзе) | pass — правка #172 не сломала соседнюю правку #150 в том же файле |
|
||||
| `node scripts/process-gate.mjs --range origin/dev..HEAD --target-ref refs/heads/issue/172-zero-divider-taper` | pass, 4 коммита, 0 предупреждений (без `--issues`, офлайн-режим — токен GitHub здесь не нужен для проверки трейлеров/веток/changelog) |
|
||||
| `git show --stat c2112db` | подтверждает оба changelog, документацию и три копии бандла в одном коммите с `User-Visible: yes` |
|
||||
|
||||
### Не прогонялось, и почему
|
||||
|
||||
- **`npm run golden:verify`.** Новая сцена `split-zero-divider-taper-dark`
|
||||
(`GOLDEN_MATRIX_VERSION` 25) не имеет baseline — проверил напрямую:
|
||||
`demo/golden/baselines/` не содержит записи `split-zero-divider-taper-dark`.
|
||||
`verify` по контракту (`demo/golden/README.md`) обязан упасть на отсутствующем
|
||||
эталоне независимо от корректности геометрии; AC8 сознательно откладывает
|
||||
принятие baseline на предрелизный Linux-гейт (`golden:accept -- --reviewed`).
|
||||
Локальный прогон дал бы только ожидаемый «missing baseline» без новой
|
||||
информации.
|
||||
- **`python -m pytest tests_backend`.** Ни один файл `custom_components/**/*.py`
|
||||
не тронут этим диффом.
|
||||
- **Performance-профили.** Не названы в AC; диф ограничен одним `if`-блоком на
|
||||
переход, асимптотика не меняется — то же заключение, что и в r1, диф не
|
||||
изменился.
|
||||
- **Полный набор из 127+ browser-smoke.** Задача — точечное изменение одной
|
||||
геометрической функции; прогнаны названный в AC смок плюс четыре смежных
|
||||
(corner-split, общая толщина стен, T/L-стыки, и — дополнительно к списку r1 —
|
||||
smoke #150, слитый той же общей функцией при ребейзе). Остальные смоки не
|
||||
используют `insetContour`/`outsetContour` в зоне, задетой этим диффом.
|
||||
|
||||
## Проверка AC1–AC11
|
||||
|
||||
Продуктовый код и тесты идентичны r1; переисполнил или перепроверил каждую
|
||||
строку самостоятельно, ссылки на r1 — только там, где вывод не может измениться
|
||||
при неизменном диффе.
|
||||
|
||||
| AC | Метод по ТЗ | Статус | Как закрыт |
|
||||
|---|---|---|---|
|
||||
| AC1 | unit | ✅ | `variable-offset contours keep a local cap…` — прогнан лично, подтверждён красным без фикса |
|
||||
| AC2 | unit | ✅ | `near-collinear zero-depth Split divider never grows a masonry taper` — матрица `outerCm ∈ {1,15,100}`, `deltaY ∈ {-5,-2.5,2.5,5}`, permutation room order/winding; прогнан лично, подтверждён красным без фикса |
|
||||
| AC3 | unit | ✅ | Полный регресс 833/833 не покраснел; читал код (`src/wall-thickness.ts:810-817` до `collinearJoint`) — при точном коллинеарном стыке новая ветка вычисляет ту же точку, что и старая (`nA===nB`), логический регресс исключён; `smoke_split_corner_wall.mjs` зелёный |
|
||||
| AC4 | unit | ✅ (см. Low-1 ниже, унаследована из r1) | Отдельного нового теста на точную AC1-fixture нет и не появилось при ребейзе (диф теста не изменился). Я предпринял независимую попытку пересчитать инвариант собственным скриптом (`innerContourForRoom` по каждой комнате в отдельности) и получил числа, не сопоставимые напрямую с методологией r1 (моя примитивная сумма per-room floor не воспроизводит точно то же сечение, что r1 мерил полосой вдоль разделителя) — не нашёл основания усомниться в выводе r1, но и не воспроизвёл его число независимо. См. «Чего не проверял» |
|
||||
| AC5 | unit | ✅ (чтением) | `splitRoomPath()` не тронута диффом; тест «rendering does not materialize or mutate saved geometry» + smoke `anglePreserved`/`renderDoesNotRewriteConfig` |
|
||||
| AC6 | smoke | ✅ | `node demo/smoke_zero_divider_taper.mjs`, лично прогнан, `dividerStaysZero`/`planHasNoTaper` true |
|
||||
| AC7 | smoke | ✅ | Тот же smoke: `planUsesCanonicalBody`, `planViewParity`, `kioskParity`, `isoUsesCanonicalBody`, `staticParity`, `lightHasNoTaper`, `renderDoesNotRewriteConfig` — все true |
|
||||
| AC8 | golden | ✅ (отложено по контракту) | Сцена в матрице v25, `test/golden-matrix.test.mjs` проверяет состав; baseline отсутствует — проверено напрямую по `demo/golden/baselines/` |
|
||||
| AC9 | unit+smoke | ✅ | 833/833 + четыре смежных/зависимых smoke зелёные (включая #150) |
|
||||
| AC10 | код-ревью | ✅ | Диф ограничен общей variable-offset геометрией `wall-thickness.ts`, ни одного renderer-specific ветвления |
|
||||
| AC11 | код-ревью | ✅ | Новых DOM-узлов/событий/таймеров/сетевых вызовов/HA-сервисов нет |
|
||||
|
||||
## Находки
|
||||
|
||||
Новых находок в этом цикле нет — диф не изменился по существу с r1, только SHA
|
||||
после ребейза. Обе находки Low из r1 остаются в силе с тем же решением
|
||||
(«снимается без правки»); переношу их сюда без повторной эскалации, чтобы не
|
||||
плодить фиктивный «новый» цикл вокруг уже закрытого вопроса.
|
||||
|
||||
### Low-1 (унаследована из r1) — AC4 не имеет отдельного исполняемого теста на fixture из АК1
|
||||
|
||||
Не изменилось с r1: отдельного unit-теста на clean-floor invariant именно для
|
||||
near-collinear нулевого разделителя по-прежнему нет. r1 закрыл разрыв прямым
|
||||
исполнением `innerContourForRoom` + `wallBodiesGeometry` и получил расхождение
|
||||
≈0,011%, идентичное точному `0°` (то есть ранее существующий
|
||||
квантование-артефакт, а не то, что фикс должен был закрыть). Моя собственная
|
||||
попытка независимо пересчитать тот же инвариант (см. AC4 выше и «Чего не
|
||||
проверял») использовала другую, более грубую методологию и не дала
|
||||
сопоставимого числа — это ограничение моей проверки, а не найденное
|
||||
расхождение с выводом r1. Диф, на котором сделан вывод r1, не изменился.
|
||||
|
||||
**Вердикт:** остаётся снятой без правки, как в r1. Не переоткрываю как новую
|
||||
находку — методологическое расхождение в моей повторной проверке не
|
||||
опровергает измерение r1 и не является само по себе дефектом кода.
|
||||
|
||||
### Low-2 (унаследована из r1) — неточная ссылка на процесс в хендоффе первого цикла
|
||||
|
||||
Касалась исходного implementation-хендоффа (цитата §11.4 не по адресу для
|
||||
пропуска named-smoke). Автор сам прогнал smoke перед вторым хендоффом
|
||||
(«Повторный хендофф после ребейза» явно перечисляет
|
||||
`node demo/smoke_zero_divider_taper.mjs → pass, все 13 проверок true`), так что
|
||||
для r2 вопрос уже неактуален практически, а не только формально.
|
||||
|
||||
**Вердикт:** снимается окончательно, без дальнейших действий.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- Ребейз не изменил продуктовую логику: `git diff origin/dev...HEAD --
|
||||
src/wall-thickness.ts` даёт тот же 25-строчный диф, что описан в r1, только с
|
||||
другим базовым SHA.
|
||||
- Правка сосуществует с #150 без конфликта поведения: обе используют одну и ту
|
||||
же общую функцию `insetContour`/`outsetContour`, `smoke_wall_thickness_transition.mjs`
|
||||
(#150) зелёный на дереве, содержащем обе правки.
|
||||
- Дисциплина «тест умеет падать» подтверждена мной лично на пересобранном
|
||||
дереве (не переиспользовано заявление r1): 2 unit-теста и named-smoke красные
|
||||
без фикса, зелёные с фиксом.
|
||||
- Три копии бандла побайтово идентичны друг другу и рабочему дереву (`git
|
||||
status --short` пуст после пересборки).
|
||||
- Трейлеры, оба changelog, `docs/WALL-THICKNESS.md`, `docs/specs/README.md` — в
|
||||
одном продуктовом коммите `c2112db` (`git show --stat`).
|
||||
- `process-gate.mjs` проходит на всём диапазоне `origin/dev..HEAD` (4 коммита,
|
||||
0 предупреждений).
|
||||
- Golden-сцена добавлена в матрицу без преждевременного baseline — проверено
|
||||
прямым просмотром `demo/golden/baselines/`, а не только чтением ТЗ.
|
||||
- Симметрия inset/outset, ограничение локального cap физической half-depth,
|
||||
независимость от порядка комнат/winding — те же гарантии, что в r1, диф не
|
||||
изменился, регресс логически исключён (см. AC3 выше).
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- Визуальный итог новой golden-сцены — baseline не существует по контракту до
|
||||
предрелиза.
|
||||
- Backend/HA harness — не затронут.
|
||||
- Полный набор из 127+ browser-smoke и `performance_smoke` — не относятся к
|
||||
этому точечному изменению; обязательны на предрелизном гейте.
|
||||
- Мобильный/touch путь Split — ТЗ фиксирует независимость сохранённой
|
||||
геометрии от типа указателя, инструмент desktop-first и не менялся этим
|
||||
диффом.
|
||||
- Независимое числовое воспроизведение AC4 (clean-floor invariant на точной
|
||||
AC1-fixture) — моя попытка пересчитать инвариант собственным скриптом
|
||||
использовала иную методологию, чем r1 (суммирование `innerContourForRoom` по
|
||||
комнате вместо измерения полосы вдоль разделителя), не дала сопоставимого
|
||||
числа и была отброшена как неубедительная, а не доведена до совпадения с
|
||||
результатом r1. Полагаюсь на измерение r1 (Low-1), поскольку диф, на котором
|
||||
оно сделано, не изменился.
|
||||
- Правильность конкретной оценки владельца (8/10 · 6/10 · P2) и легитимность
|
||||
полного трека по существу — уже подтверждены `SPEC-REVIEW-172-r1.md`, не
|
||||
предмет код-ревью.
|
||||
Reference in New Issue
Block a user