mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-02 21:01:21 +00:00
claude[bot]
@@ -0,0 +1,196 @@
|
||||
# CODE-REVIEW — issue #233, заход r1
|
||||
|
||||
- Issue: [#233](https://github.com/Matysh/houseplan-card/issues/233) — «Резайз показывает внутренние размеры, а не осевые»
|
||||
- ТЗ: `docs/specs/233-resize-inner-dimensions.md`, статус **принят зелёным на r2** (`docs/reviews/SPEC-REVIEW-233-r2.md`)
|
||||
- Ветка: `issue/233-resize-inner-dimensions`
|
||||
- Проверяемый коммит: **`abfaae3` — feat: measure resize labels between wall faces**
|
||||
(`Issue: #233`, `User-Visible: yes`)
|
||||
- Заход: r1 (код), блокирующих циклов израсходовано 0 из 4
|
||||
|
||||
## Скоуп ревью
|
||||
|
||||
Полный разбор — это первый заход кода на эту задачу, дельта совпадает с
|
||||
задачей целиком. Материал: `git diff origin/dev...HEAD` (16 файлов, единственный
|
||||
продуктовый коммит `abfaae3` поверх принятого ТЗ), плюс переписка issue #233
|
||||
(две находки спек-ревью r1 — H1 «проём сокращался бы соседями», H2 «источник
|
||||
толщин назван неверно» — обе закрыты в ТЗ на r2, зелёным).
|
||||
|
||||
## Как проверялось
|
||||
|
||||
1. Прочитан принятый ТЗ (`docs/specs/233-resize-inner-dimensions.md`) целиком,
|
||||
включая §6 (контракт `innerEdgeSpan`) и §7 (источник толщин `ownEdgeOffsets`
|
||||
/ `roomWallProfile`).
|
||||
2. Построчно сверен диф `src/wall-thickness.ts` (`innerEdgeSpan`,
|
||||
`ownEdgeOffsets`) и `src/houseplan-card.ts` (`_rszEdgeLabels`,
|
||||
`_rszInnerSpanCms`, `_rszScaleLabels`) с контрактом §6/§7.
|
||||
3. Прогнаны гейты на дереве коммита (эта среда, в отличие от песочницы автора,
|
||||
имеет Chromium — воспользовался этим и реально исполнил браузерные смоки,
|
||||
а не только код-ревью):
|
||||
- `npx tsc --noEmit` — чисто;
|
||||
- `npm test` — **1026/1026**, зелено, включая 4 новых теста `#233`;
|
||||
- `npm run build` + сверка трёх копий бандла — **md5 идентичны**
|
||||
(`5ffd65073d92c4845aecb1a4145f0948` во всех трёх местах), совпадает с тем,
|
||||
что заявил автор;
|
||||
- `node scripts/mutation-gate.mjs --check` — **87/87 `ok`**, exit 0; все три
|
||||
заявленных автором мутанта (`resize-labels-show-centreline`,
|
||||
`inner-span-shortens-a-passage`, `inner-span-reads-whole-edge-thickness`)
|
||||
реально красят свои тесты;
|
||||
- `node demo/smoke_resize_inner_dimensions.mjs` — **OK** (реально исполнен,
|
||||
не «предполагается зелёным»);
|
||||
- регрессия: `node demo/smoke_draw_wall_thickness.mjs` — OK,
|
||||
`node demo/smoke_wall_thickness_transition.mjs` — OK (оба смока, названные
|
||||
в §12 плана автотестов как обязательная регрессия).
|
||||
4. Независимая проверка мехнизма §7 (H2) вручную: собрал вне репозитория
|
||||
(`/tmp/verify233b.mjs`, импорт из `test-build/wall-thickness.js`,
|
||||
сгенерированного самим `npm test`, — файлов внутри репозитория не создавал)
|
||||
сценарий комнаты с рёбром `(0,0)–(0.4,0)`, где `setWallThickness` записан
|
||||
только на подотрезок `(0,0)–(0.2,0)` (20 см), а остаток ребра —
|
||||
без записи вовсе:
|
||||
- `thicknessCmAt(walls, [0,0], [0.4,0], pitch)` → **0** — подтверждает
|
||||
именно тот дефект, который называла находка H2 (наивный запрос по целому
|
||||
ребру ест сплит-толщину);
|
||||
- `ownEdgeOffsets(...)` → `[48, 0, 0, 0]` — корректно находит толщину
|
||||
20 см через атомарный профиль (48 единиц = половина от `20×24/5`),
|
||||
несмотря на то, что середина ребра лежит точно на границе интервалов;
|
||||
- механизм §7 работает как задумано. Это подтверждено исполнением, не
|
||||
чтением.
|
||||
|
||||
## Находки
|
||||
|
||||
Обе — в скоупе задачи, обе фиксируются в этом же раунде (без High-находок это
|
||||
жёлтый вердикт, отдельный issue не заводится — решение владельца 2026-08-19,
|
||||
#202).
|
||||
|
||||
### M1 — тест AC6b не строит сценарий, который он заявляет (и который был H2)
|
||||
|
||||
`test/wall-thickness.test.mjs:1533-1552`, тест «`ownEdgeOffsets reads the atomic
|
||||
profile, not a whole-edge lookup (#233)`»:
|
||||
|
||||
```js
|
||||
const walls = setWallThickness([], [0, 0], [1, 0], 20, pitch);
|
||||
assert.equal(thicknessCmAt(walls, [0, 0], [1, 0], pitch), 20);
|
||||
```
|
||||
|
||||
Комментарий над тестом заявляет: «половина ребра задана толщиной, половина
|
||||
нет» — но `setWallThickness` здесь вызван на **весь** осевой отрезок ребра
|
||||
комнаты `(0,0)-(1,0)` (это ребро прямоугольной комнаты `[[0,0],[1,0],[1,1],
|
||||
[0,1]]` целиком), а не на его часть. Поэтому `thicknessCmAt` по целому ребру
|
||||
корректно возвращает 20 — это НЕ тот дефект, о котором предупреждала находка
|
||||
H2 спек-ревью r1 (там `thicknessCmAt` возвращает **0** именно когда запрошено
|
||||
целое ребро против **частично** заданной толщины). Наивная реализация,
|
||||
читающая `thicknessCmAt` по целому ребру вместо атомарного профиля, **прошла
|
||||
бы этот тест** — толщина 20 совпадает в обоих подходах, раз ребро не разрезано.
|
||||
Мутант `inner-span-reads-whole-edge-thickness` (обрывает цикл `distToSeg` в
|
||||
`ownEdgeOffsets` до «всегда false») ловит тест, но по другой причине —
|
||||
проверяет, что путь через профиль вообще исполняется, а не что он даёт верный
|
||||
ответ именно на сплит-ребре.
|
||||
|
||||
Воспроизведение находки (не догадка — исполнено): сценарий с настоящим
|
||||
сплит-ребром (см. «Как проверялось», п.4) показывает, что `thicknessCmAt` по
|
||||
целому ребру даёт **0**, а `ownEdgeOffsets` — верную половину толщины 48. Это
|
||||
именно то расхождение, ради проверки которого AC6b и появился в ТЗ, но
|
||||
делаемый тест его не создаёт, поэтому не может отличить исправленный код от
|
||||
регресса к `thicknessCmAt`.
|
||||
|
||||
**Что чинить:** переписать тест, чтобы толщина была записана только на часть
|
||||
ребра (например, `setWallThickness([], [0,0], [0.5,0], 20, pitch)` на ребре
|
||||
длиной 1, либо аналогично численному примеру из «Как проверялось»), и
|
||||
проверить, что `thicknessCmAt` по целому ребру даёт 0, а `ownEdgeOffsets` —
|
||||
корректную половину толщины. Само поведение продукта уже верно (проверено
|
||||
исполнением) — правка нужна только в тесте.
|
||||
|
||||
### M2 — не хватает мутанта из принятого §11 ТЗ
|
||||
|
||||
`docs/specs/233-resize-inner-dimensions.md` §11 (принят зелёным на r2)
|
||||
перечисляет четыре мутационных стража:
|
||||
|
||||
| id | Что ломает | AC |
|
||||
|---|---|---|
|
||||
| `resize-labels-show-centreline` | подписи снова считают осевую длину | AC1, AC7 |
|
||||
| `inner-span-ignores-neighbour-thickness` | сокращение считается только по своей толщине, соседи игнорируются | AC2 |
|
||||
| `inner-span-shortens-a-passage` | правило нуля снято | AC6a |
|
||||
| `inner-span-reads-whole-edge-thickness` | источник толщин подменён на `thicknessCmAt` | AC6b |
|
||||
|
||||
`scripts/mutation-gate.mjs` (диф `abfaae3`) добавляет только три из четырёх —
|
||||
`inner-span-ignores-neighbour-thickness` отсутствует, и коммит-сообщение в
|
||||
issue прямо говорит «мутанты по задаче — все **три**», тихо уронив четвёртый
|
||||
пункт принятого контракта. Существующий unit-тест AC2 (`test/wall-thickness.
|
||||
test.mjs:1497-1501`, `mixed = [7.5, 15, 7.5, 15]`) фактически ловит путаницу
|
||||
«своя толщина вместо толщины соседа» (own=7.5 против соседей=15,15 — если бы
|
||||
код использовал `own` вместо соседей, результат отличался бы, тест проверено
|
||||
не проходит при такой подмене), но никакого мутационного стража, формально
|
||||
доказывающего это в CI, для AC2 не существует — в отличие от остальных трёх
|
||||
AC этой задачи.
|
||||
|
||||
**Что чинить:** добавить запись `inner-span-ignores-neighbour-thickness` в
|
||||
`scripts/mutation-gate.mjs`, патчащую `innerEdgeSpan` так, чтобы сокращение
|
||||
считалось по `own` вместо `offsets[edge]` в `cutAt`, и привязать её к тесту
|
||||
`innerEdgeSpan measures between wall faces, not centrelines (#233)`.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- **AC1–AC5, AC6** — доказаны юнит-тестами, численно сверены (285/385 см на
|
||||
300×400 при стенах 15 см; диагональ не равна наивному `|b-a|-o-o`; стены
|
||||
толще комнаты дают 0, не отрицательное число). Тесты реально падают на
|
||||
соответствующих мутантах (проверено прогоном гейта, не по описанию).
|
||||
- **AC6a (H1)** — правило нуля идёт первым в `innerEdgeSpan`
|
||||
(`src/wall-thickness.ts`, `if (!(own > 0)) return centre;`), перепроверено
|
||||
математически на примере из находки H1 (проём с соседями-стенами 15 см) —
|
||||
сокращения нет, полная осевая длина возвращается.
|
||||
- **AC6b (H2)** — сам механизм (`ownEdgeOffsets` → `roomWallProfile` →
|
||||
`atomicPolyForRoom` с `wallBreaks=walls`) корректен и подтверждён
|
||||
исполнением независимого сценария (см. «Как проверялось», п.4); дефект
|
||||
только в тесте, который это не показывает (M1 выше).
|
||||
- **AC7** — три подписи `_rszEdgeLabels` внутренние, габарит
|
||||
`_rszScaleLabels` — bbox внутреннего контура. Подтверждено реальным прогоном
|
||||
`demo/smoke_resize_inner_dimensions.mjs` (OK), а не только чтением кода.
|
||||
- **AC8** — площадь не изменилась (тот же путь `innerContourForRoom` +
|
||||
`floorMinusBodies`, диф не трогает вычисление площади ни в
|
||||
`_rszEdgeLabels`, ни в `_rszScaleLabels`). Подтверждено тем же смоком
|
||||
(`dragAreaStillInner`, `frameAreaAgreesWithSize`).
|
||||
- **AC9** — `abfaae3` несёт `Issue: #233` и `User-Visible: yes`; оба
|
||||
changelog (`docs/CHANGELOG.md`, `docs/CHANGELOG.ru.md`) и оба
|
||||
`docs/USER-GUIDE*.md` правлены в этом же коммите.
|
||||
- **i18n** — диф не трогает `src/i18n/*.json`, как и заявлено в ТЗ §9 (меняется
|
||||
число, не строка).
|
||||
- **Golden** — не задет по построению: `_rszLive` (место, где живут эти
|
||||
подписи) присваивается только на время активного жеста ресайза
|
||||
(`_rszEdgeDown`/`_rszMove`/`_rszDrag`) и обнуляется на `pointerup`/отмене
|
||||
(`src/houseplan-card.ts:6478,6974,8172,8224`), рендерится только внутри
|
||||
`.measurelayer` (`src/houseplan-card.ts:16433-16434`) — вне активного
|
||||
перетаскивания оверлея нет, статическая матрица golden его не видит.
|
||||
Проверено чтением кода, не исполнением `golden:verify`.
|
||||
- **Регрессия толщины стен** — `smoke_draw_wall_thickness.mjs` и
|
||||
`smoke_wall_thickness_transition.mjs` зелёные на этом дереве (см. выше),
|
||||
как того требует §12 плана автотестов ТЗ.
|
||||
- **Touch** — задача не трогает обработчики указателя (`_rszEdgeDown`/
|
||||
`_rszMove`), только текст подписи; `docs/TOUCH-SUPPORT.md` не затрагивается
|
||||
по построению (проверено чтением диффа — в нём нет изменений в
|
||||
pointer-путях).
|
||||
|
||||
## Чего не проверял и почему
|
||||
|
||||
- **`npm run golden:verify`** — не гонял. AC/диф не меняют видимый статический
|
||||
рендер (см. выше, обоснование по коду); полный прогон — предрелизный гейт
|
||||
(§8), не гейт ревью на задаче такого размера.
|
||||
- **`python -m pytest tests_backend`** — не гонял, диф не трогает
|
||||
`custom_components/**/*.py` (в дифф-стате таких файлов нет).
|
||||
- **performance-профили** — не гонял: функция чистая, вызывается на 3 ребра за
|
||||
кадр перетаскивания, не названа в AC как перфочувствительная, диф не
|
||||
трогает горячие пути рендера.
|
||||
- **`demo/smoke_*.mjs` вне названных в AC/§12** — не прогонял весь набор из
|
||||
127 смоков: диф ограничен `wall-thickness.ts` (новые чистые функции) и двумя
|
||||
методами подписей ресайза в `houseplan-card.ts`; прогнаны новый смок задачи
|
||||
плюс два регрессионных, названных в самом ТЗ (§12). Более широкой
|
||||
поверхности диф не касается.
|
||||
|
||||
## Вердикт
|
||||
|
||||
Механизм реализации корректен и совпадает с принятым ТЗ (проверено чтением и
|
||||
независимым исполнением, включая ручную проверку самого рискового места —
|
||||
сплит-толщины ребра, H2). Обе находки — про качество доказательной базы
|
||||
(тест и мутационный гейт), а не про поведение продукта: ни одна не блокирует
|
||||
(High: 0), обе в скоупе и чинятся правкой тестов/гейта в этом же раунде
|
||||
(Medium: 2) без выхода за рамки задачи.
|
||||
|
||||
**Вердикт: жёлтый · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 2 → в задаче**
|
||||
Reference in New Issue
Block a user