From 0df5db8b1c723017039bb233b33840aaf1f1a09e Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Fri, 21 Aug 2026 16:56:20 +0000 Subject: [PATCH] docs: review document for #234 Issue: #234 User-Visible: no --- docs/reviews/CODE-REVIEW-234-r2.md | 187 +++++++++++++++++++++++++++++ 1 file changed, 187 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-234-r2.md diff --git a/docs/reviews/CODE-REVIEW-234-r2.md b/docs/reviews/CODE-REVIEW-234-r2.md new file mode 100644 index 00000000..a058aed9 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-234-r2.md @@ -0,0 +1,187 @@ +# CODE-REVIEW-234-r2 + +- Issue: [#234](https://github.com/Matysh/houseplan-card/issues/234) — толщина отрезка цепочки стен расходится между превью и записью +- Этап: код-ревью (PROCESS.md §2.7) +- Заход: r2 · блокирующих циклов израсходовано **0 из 4** (r1 был зелёным, бюджет не тратил — правило #227) +- Материал: `git log --oneline origin/dev..HEAD`, `git diff origin/dev...HEAD` +- Коммит на вершине: `c8e9597` (единственный коммит диапазона, `dev`@`1e2eba9`) +- Трейлеры коммита: `Issue: #234` · `User-Visible: yes` — оба changelog правятся в этом же коммите + +## Почему разбор полный, а не по дельте + +Раунд r1 (SHA `a0878cd`) закончился зелёным вердиктом без замечаний, требующих +правки. Между r1 и этим заходом код задачи не менялся — но ветка была +**ребейзнута** на ушедший вперёд `dev` (в `dev` за это время влилась #229, +слияние коллинеарных перегородок, тоже правящая `src/houseplan-card.ts`). +PROCESS.md §2.10 и §7.2 прямо называют этот случай исключением из сокращения +по дельте: «после ребейза на ушедший вперёд `dev` это другой код». Поэтому +этот документ — полный разбор AC1…AC9, а не разбор одной строки диффа. + +Практически это означает: код резолвера и всех шести точек вызова я прочитал +и проверил заново на текущем `HEAD`, а не принял на веру утверждение «правок +кода не потребовалось» из комментария владельца. + +## Что проверил + +### 1. Диагноз и контракт по коду + +Построчно сверил `src/wall-face-graph.ts` и шесть точек в +`src/houseplan-card.ts` с контрактом §6 ТЗ (`docs/specs/234-chain-segment-thickness.md`): + +- `chainSegmentCms(segmentCount, recorded, activeCm, defaultCm)` — правило + «своя запись `> 0` → предыдущий валидный → `activeCm` → `defaultCm`» + реализовано буквально (`src/wall-face-graph.ts:78-97`); `defaultCm` + невалидный вызывающего приводится к минимальной толщине 1 см, а не к + выдуманному значению — это расширяет контракт консервативно, не меняя его. +- `wallChainSegments` лишена собственного fallback: принимает `cms[i]` как + есть (`wall-face-graph.ts:112-121`). +- Все шесть мест переведены на резолвер: + 1. превью цепочки (`houseplan-card.ts:18610-18621`); + 2. незамкнутая цепочка → перегородки, `_finishWallChain` + (`:6576-6584`); + 3. замкнутая цепочка → перегородки (`:13089-13099`); + 4. замкнутый контур → комната, `edgeCms`/`source` (`:12710-12742`); + 5. подсветка инструмента «Толщина», `_wallSourceCmAt` (`:12426-12435`); + 6. батч граней стен, `_applyWallFaceBatch` (`:12491-12497`) — шестое место, + найденное автором сверх каталога ТЗ §3; подтверждаю, что оно + действительно использовало `wallChainSegments(..., DRAW_WALL_DEFAULT_CM)` + до правки и теперь читает тот же резолвер. +- Инвариант длины: точка и её толщина пишутся одной операцией в + `_markupClick` (`:7301-7312`), `_persistActiveDraftSegment` больше не решает, + писать ли толщину (`:7426-7442`, комментарий явно называет причину). Четыре + пути чтения черновика (`:2618`, `:7243`, `:7383`, `:12817`) приведены к длине + через новый `_adoptDraftCms` (`:7432-7442`), который использует тот же + резолвер и логирует дозаполнение в `console.debug`. + +### 2. Гейты — прогнаны на HEAD (`c8e9597`) в этой сессии + +| Команда | Результат | +|---|---| +| `npx tsc --noEmit` | чисто | +| `npm test` | **1019 pass / 0 fail** | +| `npm run build` | собран | +| `cmp dist/... custom_components/.../houseplan-card.js` | совпадают побайтово | +| `cmp dist/... demo/srv/assets/houseplan-card.js` | совпадают побайтово | +| `node scripts/mutation-gate.mjs --check` | зелёный, все id, включая три новых, структурно валидны | +| `--id=chain-thickness-falls-back-to-default` | поймано 1 из 1 | +| `--id=chain-thickness-preview-diverges` | поймано 1 из 1 | +| `--id=chain-thickness-length-invariant-dropped` | поймано 1 из 1 | +| `node demo/smoke_wall_chain_thickness.mjs` | **OK** | +| `node demo/smoke_draw_wall_thickness.mjs` | **OK** (регресс §12.4 ТЗ) | +| `node demo/smoke_wall_thickness_transition.mjs` | **OK** (регресс §12.4 ТЗ, `sharedKept: true` — общие участки между комнатами не задеты) | + +В отличие от автора, у ревьюера Chromium в песочнице доступен — целевой смок +и оба регрессионных прогнаны напрямую, не со слов. + +**Тест умеет падать — проверено, а не предположено.** Временно вернул +резолверу старую подмену (`own ?? previous ?? fallbackTail` → `own ?? +fallbackTail`), пересобрал бандл, скопировал в `demo/srv/assets` и прогнал +целевой смок: он покраснел ровно на том свойстве, которое проверяет +(`gapInheritedPrevious: expected true, got false`). После проверки дерево +возвращено в исходное состояние (`git checkout -- ...`), пересборка и сверка +трёх копий подтвердили чистоту рабочей копии. + +### 3. AC — чем доказано + +| AC | Требование | Статус | +|---|---|---| +| AC1 | `chainSegmentCms` возвращает `segmentCount` строго положительных чисел на любом входе, включая явный `0` | зелёный, unit, таблица мусорных входов покрывает границу `> 0` | +| AC2 | Пропуск наследует предыдущий валидный → `activeCm` → `defaultCm` | зелёный, unit | +| AC3 | Превью и запись дают идентичный вектор на одних данных | зелёный, unit (`the preview and the writers cannot disagree`) | +| AC4 | Цепочка 30/30/(пропуск) сохраняется как 30,30,30 | зелёный, unit + смок | +| AC5 | Контур → комната сохраняет распределение превью, общие участки не задеты | смок `sharedKept: true` + прочитан код: `edgeCms[source]` берётся из того же резолвера, раскладка по интервалам (`applyWallThicknessToNewRoom`/`setWallThickness`) не тронута — вне скоупа задачи и не изменена | +| AC6 | Подсветка инструмента = записанное значение | зелёный, смок (`highlightMatchesStored`) | +| AC7 | Черновик с `segments` короче `points-1` дочиняется по правилу §6 | зелёный, unit + смок (`legacyResumedFullVector`, `legacyGapsInheritedThirty`) | +| AC8 | Точка не добавляется без валидной толщины; длины согласованы | **проверено чтением, не исполнением**: атомарная запись в `_markupClick` структурно исключает расхождение; отдельного сквозного теста последовательности кликов нет — граница признана автором и приемлема, т.к. поведение выражено на уровне одного метода, а не координации двух | +| AC9 | Обе записи changelog в том же коммите | в диффе `c8e9597`, подтверждено `git show --stat` | + +### 4. Взаимодействие с #229 (ребейз) + +Автоматическое слияние `src/houseplan-card.ts` и `scripts/mutation-gate.mjs` — +затронутые участки не пересекаются (резолвер `chainSegmentCms` и `spaceMergeGeometry` +из #229 работают на разных строках). Косвенное подтверждение: `npm test` даёт +1019 тестов (было 989 на `a0878cd`, `dev` принёс +30 своих), все зелёные; +регресс `smoke_wall_thickness_transition.mjs` (тоже смежный с геометрией стен) +зелёный. Отдельно проверил, что смок `smoke_wall_chain_thickness.mjs` строит +неколлинеарную цепочку (прямой угол между вторым и третьим отрезком), то есть +автослияние коллинеарных перегородок (#229) в этом сценарии не участвует — +это не создаёт риска для AC4, но и не является для них доказательством +совместной работы; совместная работа проверена лишь тем, что полный набор +юнит-тестов (включающий тесты #229) зелёный. + +## Закрытие раунда r1 + +r1 (SHA `a0878cd`) — вердикт зелёный, единственная находка Low, снятая с +записью, без обязательной правки. + +| Находка r1 | Чем закрыта | Где видно | +|---|---|---| +| Low: недостижимая ветка `if (cm == null) { … return; }` в `_markupClick`, дублирующая проверку `_canAppendRoomDraftPoint` | Не требовала правки в r1 (снята с записью: поведения не меняет, диапазон 1…100 закрыт UI). Ребейз этот код не тронул — строки идентичны | `src/houseplan-card.ts:7301-7308` (проверка `_canAppendRoomDraftPoint` на `:7170-7171`) — прочитано на `HEAD`, содержимое совпадает с описанием в комментарии r1 | + +Новых находок в этом заходе нет. + +## Унаследовано из r1 + +Формально это полный разбор (см. раздел выше про причину), поэтому ничего не +принято «на слово» — каждый пункт ниже перепроверен на текущем `HEAD`, а не +скопирован из документа r1: + +- Диагноз шести точек и их перевод на `chainSegmentCms` — перечитан построчно + на `c8e9597`, совпадает с описанием r1 (`docs/reviews/CODE-REVIEW-234-r1.md`, + SHA `a0878cd`) и с ТЗ §3/§6. +- AC1–AC7, AC9 — тесты и смоки, зелёные ранее и сейчас, прогнаны заново. +- AC8 — тот же способ доказательства («прочитано, не исполнено»), что и в r1; + структура кода не изменилась. +- Продуктовая рамка и touch-классификация (`not exposed`) — унаследованы из + ревью ТЗ (`docs/reviews/SPEC-REVIEW-234-r2.md`), рабочий код их не касается. + +Единственное, что действительно не перепроверялось повторно, — сам текст ТЗ: +ребейз его не менял, а спек-ревью r1/r2 уже дало зелёный вердикт по контракту. + +## Находки + +Нет High. Нет Medium. Low из r1 — не новая, подтверждена присутствующей и +безвредной (см. таблицу закрытия выше), новой правки не требует. + +## Что проверено и корректно + +- Единственный резолвер толщины, все шесть точек записи/чтения/подсветки на + нём, `wallChainSegments` без собственного fallback — контракт §6 ТЗ + реализован буквально. +- Инвариант длины `_draftSegmentCms`/`_path` закреплён атомарной записью и + дозаполнением при чтении черновика. +- Гейты: typecheck, полный набор юнит-тестов, сборка со сверкой трёх копий + бандла, целевой и оба регрессионных смока, точечные прогоны трёх новых + мутаций — все зелёные, и целевой смок проверен на способность падать лично. +- Трейлеры и changelog (RU+EN) в том же коммите. +- i18n не затронут (строки не менялись, `src/i18n/*.json` в диффе нет) — + соответствует §9 ТЗ. +- Ребейз не создал конфликта с #229 на уровне поведения: полный прогон тестов + зелёный, затронутые участки не пересекаются. + +## Чего не проверял и почему + +- **`npm run golden:verify`** — не прогонял. Diff не меняет ни одной формулы + рендера (геометрия, цвет, слои), только источник значения толщины, + подаваемого в уже существующие формулы; то же обоснование, что в r1. +- **`python -m pytest tests_backend`** — не прогонял, `custom_components/**/*.py` + не тронут. +- **Полный прогон 127 смоков** — не прогонял, только целевой плюс два названных + в плане автотестов ТЗ (§12.4) регресса на смежных поверхностях (толщина при + рисовании, переход толщины при сплите). Остальные смоки не касаются + тронутых поверхностей (рисование цепочки, толщина, черновики). +- **Полный/дорогой прогон `mutation-gate`** (весь набор мутаций, а не + структурный `--check` плюс три точечных) — не прогонял, это предрелизный + гейт (PROCESS.md §8), не гейт ревью. +- **Perf-профили** — не прогонял: в AC они не названы, резолвер чистый и + вызывается на кадр рисования на массиве ограниченной длины (`MAX_DRAFT_POINTS`), + риск не следует из диффа. + +## Вывод + +Ребейз не изменил код задачи и не создал конфликта с #229. Все шесть точек +резолвера, инвариант длины и AC1…AC9 проверены на текущем `HEAD` — не унаследованы +слепо. Единственная находка r1 (Low) осталась на месте, безвредна, новой правки +не требует. Новых находок нет. + +**Вердикт: зелёный.**