diff --git a/docs/reviews/CODE-REVIEW-228-r1.md b/docs/reviews/CODE-REVIEW-228-r1.md new file mode 100644 index 00000000..e0f949e7 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-228-r1.md @@ -0,0 +1,329 @@ +# CODE-REVIEW-228-r1 + +- Issue: [#228](https://github.com/Matysh/houseplan-card/issues/228) — надёжное рисование стен и операции с готовым контуром +- ТЗ: `docs/specs/228-plan-drawing-problems.md` (ревью ТЗ зелёное, r1, #228) +- Ветка: `issue/228-plan-drawing-problems` +- Коммит реализации: `2aaef12` (`fix: make plan drawing fail closed`, `Issue: #228`, `User-Visible: yes`) +- Заход ревью: r1 · блокирующих циклов израсходовано 0 из 4 (первый заход, дельта не разбирается — весь диапазон `origin/dev...HEAD`) +- Ревьюер: Claude (код-ревью), сессия без контекста реализации + +## 1. Скоуп проверки + +Диапазон `git log --oneline origin/dev..HEAD`: три коммита — +`dca4ed2` (ТЗ), `7eedd7e` (ревью ТЗ), `2aaef12` (реализация). Весь +продуктовый код и тесты сосредоточены в одном коммите `2aaef12`. +Диапазон `git diff origin/dev...HEAD --stat`: 31 файл, +2600/-356, +из них продуктовый код — `src/houseplan-card.ts` (+397/-90), +`src/plan-snap-overlay.ts` (+153/-25), новые `src/wall-face-repair.ts` +(+150) и `src/room-deletion.ts` (+122), `src/wall-face-graph.ts` (+36), +`src/logic.ts` (+12), `src/styles.ts` (+27); тесты — 146 строк в +четырёх файлах; один новый smoke (`demo/smoke_plan_drawing_repairs.mjs`, +143 строки) плюс точечная правка существующего +(`demo/smoke_unified_wall_tool.mjs`); i18n en/ru; семь файлов +документации; три синхронные копии бандла. + +Прочитаны перед разбором: `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md`, +тело issue #228 и все семь комментариев (аналитика → продуктовые +вопросы → решения владельца → «ТЗ готово» → зелёное ревью ТЗ → «Взял» → +хендофф разработчика), полный текст +`docs/specs/228-plan-drawing-problems.md` (все 17 AC), затронутые +канонические документы (`CANVAS.md`, `ARCHITECTURE.md`, +`WALL-THICKNESS.md`, `TOUCH-SUPPORT.md`, `USER-GUIDE.ru.md`). + +## 2. Как проверялось — гейты + +Объём гейтов соразмерен задаче (§8 PROCESS.md): дешёвые гейты +прогнаны полностью, тяжёлые — по необходимости, определяемой diff'ом +и AC. + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | зелёный, без вывода | +| Unit | `npm test` | `# tests 1031 / # pass 1031 / # fail 0` | +| Build | `npm run build` | зелёный, `dist/houseplan-card.js` собран за 12.6s | +| Синхронность бандла | `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` после сборки чист | +| Named smoke | `node demo/smoke_plan_snap_overlay.mjs` | все 34 проверки `true`, `OK` | +| Named smoke | `node demo/smoke_unified_wall_tool.mjs` | все 19 проверок `true`, `OK` | +| Named smoke | `node demo/smoke_plan_drawing_repairs.mjs` | все 14 проверок `true`, `OK` | + +Эти три smoke named в §14.2 ТЗ и покрывают ровно те поверхности, +которые задевает diff. Расхождение с хендоффом автора (у него +«1030 pass, 1 skipped», у меня «1031 pass, 0 fail») не влияет на +вердикт — оба прогона зелёные, разница объясняется средой (моя +локальная копия не показывает ни одного skipped теста); я не стал +разбирать её отдельно, так как результат в обоих случаях «всё +проходит». + +**Чего не прогонял и почему:** + +- `npm run golden:verify` — diff не меняет ни один *существующий* + видимый кадр (новые визуальные элементы — `active-axis`, + `active-vertex`, `wall-repair-preview`, `.conflict` узел, + `hp-dialog` удаления комнаты — по коду не рендерятся, пока + инструмент неактивен; `viewHasNoEditorRepairChrome` в smoke это + подтверждает). Полная golden-матрица — предрелизный гейт (§14.3 + ТЗ, §8 PROCESS.md), не гейт код-ревью. +- `python -m pytest tests_backend` — ни один файл + `custom_components/**/*.py` не тронут; ТЗ §16 прямо фиксирует, что + Python/schema изменений не ожидается, и это подтверждено diff'ом. +- Полный smoke-набор (127 файлов) и performance-профили — задача не + задевает все поверхности; названные в АС и связанные с diff смоки + прогнаны выше. Это предрелизный гейт по действующему решению + владельца (§8, §11.4). + +## 3. Разбор по AC + +Ревью кода отвечает за вопрос «оно вообще работает»; там, где нет +автотеста, ниже явно написано «проверено чтением, не исполнением». + +- **AC1** (активная ось/узел) — `smoke: activeSegmentShowsAxisAndNode` + зелёный; код рендерит `.active-axis`/`.active-vertex` только когда + снап не выдал candidate/conflict (houseplan-card.ts:18970-18977, + `_drawSvg`), что не дублирует активный snap-маркер. Проверено + автотестом. +- **AC2** (неоднозначные endpoints) — `test/plan-snap-overlay.test.mjs` + покрывает ambiguous/resolved-после-zoom; `smoke: + closeEndpointsFailClosed` зелёный; click-путь + (`_markupClick`, houseplan-card.ts:7286-7295) не пишет точку и + показывает toast. Проверено автотестом. +- **AC3** (strict Shift) — `resolveStrictPlanSnap` в + `plan-snap-overlay.ts` реализует forward-ray проекцию для endpoint и + line/ray intersection; unit-тест «strict Shift accepts only points…» + и smoke `strictShiftUsesExactRayIntersection` зелёные. Прочитан код + геометрии луча (`selectedRay`, `pointOnForwardRay`) — пересечение + считается через параметрическое уравнение, non-Shift путь + (`resolvePlanSnapResult`) не тронут кроме ambiguity guard, как и + требует §8.3.5. Проверено автотестом. +- **AC4** (угловая подпись) — `isExact45Vector` (logic.ts) заменяет + широкий `is45(deg, 0.5)`; вызывается с geometry epsilon + `this._gridPitch * 0.0002`, тем же порядком величины, что и везде в + файле. Smoke `angleColourMatchesActualVector` явно проверяет + 90,1° как красный. Проверено автотестом. +- **AC5/AC8** (комната из готовой области) — `findWallFaceAtPoint` + (wall-face-graph.ts) исключает точки на границе и выбирает + наименьшую площадь; `_offerExistingWallFace` вызывается только при + пустом `_path`, без snap-candidate и без Shift (houseplan-card.ts:7295). + Smoke проверяет `existingFaceOffersRoom`, + `keepExistingFaceIsNoop`, `shiftBypassesExistingFaceOffer`, + `createExistingFaceKeepsPartitions` (после Create партиции остаются + 4 — существующий контур не тронут). Проверено автотестом. +- **AC6/AC7** (repair ≤2 см, атомарность, негативная матрица) — + `wall-face-repair.ts` реализован как чистая функция, + immutable-инвариант проверен тестом (`structuredClone` + snapshot-сравнение). Позитив (1,2 см и ровно 2,0 см → repair; 2,01 → + none), негатив «multiple valid closures fail closed» и «room vertex + never moves» (assert на `!sourceKey.startsWith('static:room|')`) + покрыты unit-тестами; commit-only-with-Create и bit-equivalent + Cancel/Keep — smoke `repairCommitsOnlyWithRoom`. **Не покрыто**: + см. находку M1 ниже — hosted-opening negative case из + ТЗ §14.1 п.8 не имеет ни unit, ни smoke доказательства, хотя AC7 + явно называет `unit + smoke` способом доказательства. +- **AC9** (диалог удаления) — `_renderRoomDeleteDialog` использует + `hp-dialog` (не `confirm()`), primary «Keep walls», danger «Delete + walls», `@hp-close` на Cancel/X/Escape. Smoke + `deleteUsesAccessibleChoiceDialog` подтверждает отсутствие + `confirm` и наличие `hp-dialog` в DOM. `hp-dialog` — переиспользуемый + компонент (тот же, что и `_renderPartitionDeleteDialog`), его + focus-trap/Escape-контракт не часть этого diff — проверено чтением, + не переисполнено отдельно. +- **AC10/AC11** (Keep/Delete walls, эксклюзив/shared/hosted) — + `room-deletion.ts` — чистая функция; unit-тесты покрывают + reuse compatible masonry, virtual/shared/zero-thickness исключение, + exclusive rehost vs shared/hosted survival, + `parameterOnPartition` для двух направлений полигона. Smoke + `keepWallsMaterializesAndRehosts` и + `deleteWallsCascadesExclusiveOpening` зелёные. Проверено чтением + вызывающего кода `_confirmRoomDelete`: capacity-проверка + (`MAX_PARTITIONS`) выполняется до `_geometrySnapshot()`/мутации — + соответствует «fail before mutation». Проверено автотестом плюс + чтением оркестрации. +- **AC12** (один Undo/атомарность) — `_recordGeometry(...)` вызывается + один раз в конце `_confirmRoomDelete` и один раз в конце + `_applyWallFaceBatch`; проверка `_validateWallRepair` выполняется до + захвата `before`-снапшота и до записи в `sp`. Проверено чтением, не + исполнением (нет отдельного Undo/Redo unit-теста для этого пути, но + структура кода идентична уже принятому паттерну `_applyWallFaceBatch` + из #173, который эту гарантию уже несёт). +- **AC13** (существующие рендер-потребители не регрессируют) — diff не + трогает Iso/Glow/sun рендер; новые SVG-элементы гейтятся текущим + инструментом/режимом. `viewHasNoEditorRepairChrome` подтверждает + отсутствие чрезмерной рендер-цепочки в View. Проверено чтением плюс + smoke. +- **AC14** (touch/gesture safety) — код не вводит новых touch-путей; + `_offerExistingWallFace` вызывается из того же `_markupClick`, что + уже проходит через существующий suppressed-click/pointercancel guard + (не тронут этим diff). Явного нового targeted-smoke на touch-жесты + для repair/existing-face нет, но `smoke_plan_snap_overlay` + (`panPinchCancelDoNotCommit`, не в этом diff, но покрывает тот же + путь) и общий контракт `_markupClick` дают достаточное покрытие + чтения. Проверено чтением, не исполнением. +- **AC15** (perf) — `findWallFaceAtPoint`/`planWallFaceRepair` + вызываются только из `_offerExistingWallFace` (idle click) и + `_offerWallFaces` (после принятого сегмента); ни один вызов не стоит + в `_svgPointerMove`/hover-пути (тот использует только + `resolvePlanSnapResult`/`resolveStrictPlanSnap`, O(E) над кэшированной + геометрией). Проверено чтением, не исполнением; performance-профиль + — предрелизный гейт по ТЗ §14.3. +- **AC16** (backend/schema) — `custom_components/**/*.py` не тронут; + новых persisted-полей нет, `partitions`/`openings` используют + существующую схему (`cm`, `host.kind: 'partition'`). Проверено + чтением diff'а. +- **AC17** (гейты, i18n, changelog, документация) — см. таблицу гейтов + выше; en/ru ключи парные (`btn.delete_room_keep_walls` и т. д.); + `docs/CHANGELOG.md`/`docs/CHANGELOG.ru.md` правлены в том же + коммите `2aaef12` с `User-Visible: yes`; `docs/CANVAS.md`, + `docs/ARCHITECTURE.md`, `docs/WALL-THICKNESS.md`, + `docs/TOUCH-SUPPORT.md`, `docs/USER-GUIDE.ru.md`, `docs/TESTING.md`, + `docs/STATUS.md` обновлены и соответствуют реализации (см. §4 ниже + про место, где текст документации разошёлся с точным поведением + кода). Проверено чтением всех перечисленных диффов. + +## 4. Находки + +### M1 (Medium, в скоупе) — негативный сценарий «hosted opening» не доказан автотестом + +**Файл:** `src/houseplan-card.ts:12673-12677` (`_validateWallRepair`), +соотнесено с `docs/specs/228-plan-drawing-problems.md` AC7 и §14.1 п.8. + +**Описание:** AC7 явно требует доказательства `unit + smoke` для +негативной матрицы, включающей «invalid hosted opening»: repair, +который сдвинул бы partition, на которой уже висит проём, обязан быть +отклонён. Guard в коде существует +(`if (proposal.sourceKey.startsWith('static:partition|')) { … if +(openings.some(host.id === id)) return false; }`) и по чтению кода +верен (правильно парсит `static:partition||…` через уже +существующий формат ключа из `plan-snap-overlay.ts:sourceKey`). Но ни +`test/wall-face-repair.test.mjs` (проверяет только чистую геометрию, +не знает о проёмах), ни `demo/smoke_plan_drawing_repairs.mjs` +(единственный `opening` в смоке участвует только в сценариях удаления +комнаты, не в сценарии 2-см repair) не воспроизводят этот случай. + +**Как проявится:** если этот guard будет случайно ослаблен или удалён +в будущей правке (например, при рефакторинге `_validateWallRepair`), +ни один тест не покраснеет — repair сможет сдвинуть стену, на которой +висит проём, произведя проём с некорректным host/geometry, то есть +нарушив инвариант ТЗ «Repair не остаётся вычисляемой ‘почти связью’» и +«Проём не остаётся orphan». Разбор чтением не заменяет здесь автотест, +потому что сам процесс требует именно `unit + smoke` для этой строки +AC7. + +**Что нужно:** один unit-тест на уровне `houseplan-card` (или +эквивалентный целевой smoke-сценарий в +`demo/smoke_plan_drawing_repairs.mjs`) с существующей room-wall или +independent-partition, несущей hosted opening в зоне ≤2 см от +незамкнутого face, подтверждающий, что `_offerExistingWallFace`/ +`_applyWallFaceBatch` не создают repair либо отклоняют его на Create с +понятным сообщением. + +### M2 (Medium, в скоупе) — неоднозначный «gap > 2 см» диагностируется как единственный + +**Файл:** `src/houseplan-card.ts:7675-7687` (`_offerExistingWallFace`), +соотнесено с `docs/specs/228-plan-drawing-problems.md` §8.6, последний +абзац. + +**Описание:** ТЗ дословно: «Если gap больше 2 см, но около click +существует **единственный** диагностируемый разрыв в screen +snap-range, он подсвечивается и room dialog не открывается. +**Неоднозначный** или недиагностируемый открытый набор остаётся +обычным Walls flow». Код второй (более широкой) диагностики не +различает `diagnostic.kind === 'repair'` и `diagnostic.kind === +'ambiguous'`: + +```ts +if (diagnostic.kind !== 'none') { + this._wallRepairDiagnostic = diagnostic.kind === 'repair' + ? diagnostic.proposal : diagnostic.proposals[0] || null; + this._showToast(this._t('toast.wall_repair_too_large')); + return true; +} +``` + +При `kind === 'ambiguous'` код всё равно подсвечивает один +произвольный `proposals[0]`, показывает toast и **перехватывает клик** +(`return true`), вместо того чтобы вернуть `false` и позволить обычный +Walls flow (начать новую цепочку с этой точки), как того требует +дословный текст ТЗ. + +**Как проявится:** три-четыре стены, образующие незамкнутый контур с +разрывом 3 см, у которого в пределах 12 CSS px существует более одного +структурно равноценного способа замкнуть face (например, две разные +комбинации independent-стен) — обычный клик внутри контура вместо +обычного начала новой стены покажет «Разрыв больше 2 см» и подсветит +один из вариантов, будто это единственный. Пользователь не может +продолжить рисовать стену с этой точки без дополнительного действия +(Shift+click), хотя контракт обещает ordinary flow. + +**Что нужно:** при `diagnostic.kind === 'ambiguous'` в этой ветке +вернуть `false` (не перехватывать клик, не подсвечивать, без toast), +как это уже сделано для случая `result.kind === 'ambiguous'` в +основной (≤2 см) ветке — там как раз есть выделенный +`toast.wall_repair_ambiguous`, здесь дополнительной обработки не +требуется вовсе, только не путать `ambiguous` с `repair`. + +Обе находки Medium и **в скоупе задачи** (AC7 и §8.6 — прямая часть +ТЗ #228): по решению владельца (2026-08-19, #202) отдельный issue не +заводится, правка делается в этом же issue, фикс проходит повторный +цикл ревью. + +## 5. Что проверено и корректно (сверх раздела 3) + +- Ключи `static:*`/`active:*`/draft `id` формата `:` + строятся в одном месте (`plan-snap-overlay.ts:sourceKey`, + `houseplan-card.ts:_activeWallSourceKey`) и корректно парсятся + обратно в `_applyWallRepair`/`_validateWallRepair` для всех трёх + веток (`active:`, `static:partition|`, `static:draft|`) — трассировка + индекса `/:(\d+)$/` и `lastIndexOf(':')` соответствует формату + генератора ключа, включая случай `endpoint === 'b'` (сдвиг индекса + на +1). Не нашёл рассинхронизации. +- `stableEndpointMover`/`movable()` в `wall-face-repair.ts` корректно + исключает пары, где оба конца — room-vertex (обе стороны + immovable), и не позволяет room выступать «двигателем» даже когда + он геометрически «удобнее» (grid-canonical) — проверено чтением и + подтверждено тестом на `!sourceKey.startsWith('static:room|')`. +- Капасity-проверка в `_confirmRoomDelete` считает именно новые (не + переиспользуемые) партиции через `Set` по `interval.key` **до** + снятия геометрии — соответствует «Capacity … fail before mutation» + (ТЗ §8.7/инвариант 8). +- Три копии бандла после чистой пересборки побайтно идентичны, рабочее + дерево чисто после сборки — обязательное условие §8 AGENTS.md + выполнено этим прогоном. +- i18n: каждый новый en-ключ имеет ru-пару и наоборот (`btn.*`, + `history.*`, `confirm.*`, `toast.*`), термины «независимая стена», + «увеличить масштаб» соответствуют `docs/USER-GUIDE.ru.md`. +- Документация обновлена в затронутых канонических файлах и не + противоречит коду ни в одном прочитанном месте, кроме находки M2, + где именно **код** расходится с уже верно написанным текстом ТЗ (не + наоборот). + +## 6. Чего не проверял + +- Golden/визуальную матрицу (active axis, conflict-узел, red repair + preview, delete-dialog) — предрелизный гейт, не гейт код-ревью; + новые CSS-классы `active-axis`/`.plan-snap-node.conflict`/ + `.wall-repair-preview` просмотрены только по исходнику styles.ts, не + по скриншоту. +- Полный HA backend harness — не тронут ни один `.py`-файл, пропуск + обоснован ТЗ §16. +- Performance-профиль 60-room/60-partition (AC15) — не запускал + профилировщик, вывод сделан чтением вызовов (см. §3, AC15). + Предрелизный гейт по ТЗ §14.3. +- touch-специфичные жесты (pinch/pan/pointercancel) именно для + existing-face/repair путей — не находил отдельного smoke-сценария + под палец/эмуляцию тача для новой функциональности; полагался на то, + что она проходит через тот же `_markupClick`, что и остальной Walls + flow, чей gesture-guard не тронут этим diff. +- Многопользовательский конкурентный сценарий (структурный + fingerprint при открытом диалоге удаления комнаты, пока другой + клиент меняет геометрию) — не воспроизводил вручную; по чтению кода + `_confirmRoomDelete` план заново строится из текущего `space` в + момент подтверждения, а не из снимка на момент открытия диалога, что + делает застаревание маловероятным, но это не проверено тестом. + +## 7. Вердикт + +Обе находки — Medium, в скоупе задачи, без High. По §2.7/§4 +PROCESS.md это жёлтый вердикт: задача возвращается автору на правку в +рамках issue #228, фикс проходит повторный цикл код-ревью (бюджет +цикла — 1 из 4, поскольку жёлтый вердикт цикл расходует). + +Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 2 → в задаче