diff --git a/docs/reviews/CODE-REVIEW-233-r1.md b/docs/reviews/CODE-REVIEW-233-r1.md new file mode 100644 index 00000000..3eb2c946 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-233-r1.md @@ -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 → в задаче**