docs: review document for #294

Issue: #294
User-Visible: no

docs/reviews/CODE-REVIEW-294-r1.md

@@ -0,0 +1,249 @@
+# CODE-REVIEW-294-r1
+
+- **Issue:** #294 — «Рисование стен: Esc отцепляет текущую цепочку без удаления последнего отрезка»
+- **Заход:** r1 (первый код-ревью-цикл; ревью ТЗ прошло r1→r2 отдельно и не считается в этом бюджете)
+- **Блокирующих циклов:** 0/4 израсходовано до этого захода
+- **Ревьюемый коммит:** `32e8de4f` («feat: detach wall chain on Escape»), ветка `issue/294-wall-esc-detach`
+- **Материал:** `git log --oneline origin/dev..HEAD`, `git diff origin/dev...HEAD`
+- **ТЗ:** `docs/specs/294-wall-esc-detach.md` (редакция r2, принята зелёным SPEC-REVIEW-294-r2)
+
+## Скоуп изменения
+
+Диапазон `origin/dev..HEAD` — пять коммитов: аналитика/ТЗ/два ревью ТЗ уже
+приняты отдельно, реализация — один коммит `32e8de4f`. Файлы этого коммита:
+
+- `src/houseplan-card.ts` — 1 содержательная строка: `Escape` в ветке
+  `this._tool === 'draw' && this._path.length` теперь вызывает
+  `this._finishWallChain()` вместо `this._undoPoint()`;
+- `src/i18n/en.json`, `src/i18n/ru.json` — `markup.hint_points` различает
+  Esc/Ctrl+Z;
+- `demo/smoke_unified_wall_tool.mjs` — новые regression-сценарии через реальный
+  `window keydown` и `stage` click;
+- `docs/USER-GUIDE.md`, `docs/USER-GUIDE.ru.md` — все шесть поверхностей,
+  перечисленных в §5 ТЗ и требуемых AC7;
+- `docs/CHANGELOG.md`, `docs/CHANGELOG.ru.md` — бюллетень в том же коммите;
+- `dist/houseplan-card.js`, `custom_components/houseplan/frontend/houseplan-card.js` —
+  синхронизированный бандл (класс D, сгенерировано `bundle:sync`).
+
+Трейлеры коммита: `Issue: #294`, `User-Visible: yes` — оба changelog в этом же
+коммите. Корректно.
+
+## Как проверялось
+
+### Гейты
+
+| Гейт | Статус | Примечание |
+|---|---|---|
+| `npx tsc --noEmit` | **pass** | без вывода |
+| `npm test` | **pass** | 1251 tests, 1250 pass, 1 skip, 0 fail — совпадает с хендоффом |
+| `npm run build` + `cmp dist ↔ frontend` | **pass** | сборка идентична закоммиченным `dist/` и `custom_components/.../frontend/` |
+| `npm run bundle:sync` (для стенда) | выполнен локально ревьюером | `demo/srv/assets/houseplan-card.js` не коммитится (#255), пересобран для прогона смоков |
+| `node scripts/check-docs.mjs` | **FAIL** | см. Находку 1 — обязателен, диф трогает `src/**` |
+| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | выполнен | вывод ниже |
+| `node demo/smoke_unified_wall_tool.mjs` (прямое совпадение) | **FAIL** (2 из 26) | см. Находку 2 |
+| `node demo/smoke_wall_chain_merge.mjs` (прямое совпадение) | **pass** | 9/9, без регрессий |
+| `node demo/smoke_wall_chain_thickness.mjs` (прямое совпадение) | **pass** | OK |
+| `npm run golden:verify` | не прогонялся | см. «Чего не проверял» |
+| `npm run invariants` / geometry model | не прогонялся отдельно | см. «Чего не проверял» |
+| `python -m pytest tests_backend` | не прогонялся | диф не трогает `custom_components/**/*.py` |
+
+Вывод `scripts/smoke-select.mjs --base origin/dev --head HEAD`:
+
+```
+Изменено файлов src/**: 1 · символов проекта на изменённых строках: 2
+Матрица: 184 смоков · порог «широкого» символа: больше 36 смоков
+
+Прямое совпадение (3):
+  demo/smoke_unified_wall_tool.mjs
+    ← _finishWallChain
+  demo/smoke_wall_chain_merge.mjs
+    ← _finishWallChain
+  demo/smoke_wall_chain_thickness.mjs
+    ← _finishWallChain
+```
+
+Решение по строке: все три «прямых совпадения» прогнаны (таблица выше).
+Остальные 181 смоков не запускались — задача меняет одну ветку клавиатурного
+обработчика существующего единого инструмента «Стены», не трогает face
+detection, split, decor, resize, touch и т.д.; полный прогон матрицы — гейт
+предрелиза (§8), не этого ревью.
+
+### AC — построчно
+
+- **AC1** (`smoke`): доказан кодом и частично смоком.
+  `_finishWallChain()` при `this._path.length >= 2` конвертирует сегменты в
+  `partitions` той же функцией, что и при смене инструмента, удаляет
+  `room_draft`, очищает `_path/_activeDraftId/_draftSegmentCms/_closingWallCm`
+  и снэп-hover (`src/houseplan-card.ts:6720-6774`). Ассерт
+  `escapeFinishesWithoutDeletingSegments` в смоке зелёный. **Проверено чтением
+  + прогоном смока.**
+- **AC2** (`smoke`): код проверен чтением и подтверждён отдельным отладочным
+  прогоном, но штатный смок **красный** — см. Находку 2. Причина красного —
+  дефект теста (сравнение чисел с плавающей точкой), не продукта: код разбора
+  клика падает в ту же `_markupClick` → `_path.length === 0` ветку → новая
+  точка становится первым узлом чистого пути, а `_draftEndAt` ищет только в
+  `space.room_drafts`, который после `Esc`-finish пуст, поэтому старая точка
+  `C` не подхватывается (`src/houseplan-card.ts:7565-7591`, `7621-7636`).
+  **AC2 подтверждён чтением кода и точечной проверкой (см. Находку 2), но не
+  зелёным автотестом — до фикса теста это открытый пункт.**
+- **AC3** (`smoke`): `escapeClearsOnlyTransientFirstPoint` зелёный; ветка
+  `this._path.length < 2` в `_finishWallChain` очищает состояние без записи
+  `room_draft`/`partition`/history (`6723-6730`). Подтверждено.
+- **AC4** (`smoke`): `ctrlZContractStillRemovesLastSegment` зелёный;
+  `Ctrl/Cmd+Z` не тронут этим диффом и по-прежнему идёт через
+  `_undoActiveDraftPoint`/`_undoPoint` (`2588-2608`). Подтверждено.
+- **AC5** (`smoke`): `firstEscapeOnlyCancelsRoomDialog` и
+  `secondEscapeFinishesRestoredDraft` зелёные; в `_onKey` проверка
+  `if (this._roomDialog) { …; return; }` (`2623-2627`) стоит раньше ветки
+  `draw`/`_path.length` (`2628-2635`) и не тронута диффом. Подтверждено.
+- **AC6** (`smoke` + чтение): `rejectedFinishKeepsActiveDraft` зелёный;
+  проверка `MAX_PARTITIONS` в `_finishWallChain` (`6739-6742`) выполняется
+  раньше любой мутации `_path`/`sp.partitions`/history, поэтому `false` не
+  теряет состояние. Подтверждено.
+- **AC7** (`unit` + ревью кода): все шесть поверхностей из §5 ТЗ построчно
+  сверены с `docs/USER-GUIDE.md`/`docs/USER-GUIDE.ru.md` — см. раздел «Что
+  проверено и корректно». `test/i18n-parity` часть `npm test` зелёная.
+  Подтверждено чтением, без старых противоречащих формулировок.
+- **AC8** (`unit` + `build`): `tsc`, `npm test`, `npm run build`+bundle parity
+  — все pass (таблица гейтов). Подтверждено исполнением.
+
+## Находки
+
+### Находка 1 (Medium, в скоупе — правится в этом issue)
+
+**Обязательный гейт `node scripts/check-docs.mjs` красный на ревьюемом SHA.**
+
+```
+ERROR screenshot source fingerprint is stale; run npm run build && node demo/docs/capture.mjs
+```
+
+Воспроизведение: на `origin/dev` (`8c036564`) тот же скрипт зелёный —
+`Documentation checks passed (7 files, 10 external links)`. На `32e8de4f`
+(этот диф) — красный, потому что `docs/images/screenshots.json.sourceFingerprint`
+считается по всему `src/**` (#245/#246), а `src/houseplan-card.ts` изменён, и
+пересъёмка/принятие скриншотов в этом коммите не выполнены.
+
+Это ровно тот гейт, для которого правила ревью прямо говорят «выбирать нечего»
+и напоминают цену пропуска: в #230 и #234 его пропустили, и `dev` простоял с
+красным job `docs` до следующей задачи (#237). Оставлять его красным в этом
+ревью значило бы повторить #230/#234.
+
+**Правка:** прогнать job «Docs screenshots» (`workflow_dispatch`), принять
+результат `npm run docs:accept -- --reviewed --from=<артефакт>` и закоммитить
+обновлённый `docs/images/screenshots.json` (+ при необходимости PNG) вместе с
+задачей, либо явно объяснить в хендоффе, почему для этого текстового изменения
+пересъёмка не нужна (в текущем виде это не объяснено и гейт просто красный).
+
+### Находка 2 (Medium, в скоупе — правится в этом issue)
+
+**Новый таргетированный смок `demo/smoke_unified_wall_tool.mjs` падает на
+двух ассертах, которые как раз доказывают AC2.**
+
+```
+FAILED (2):
+  - nextClickStartsIndependentChain: expected true, got false
+  - secondClickCreatesOnlyIndependentSegment: expected true, got false
+```
+
+Воспроизведение: `npm run bundle:sync && node demo/smoke_unified_wall_tool.mjs`
+(бандл стенда не коммитится — #255 — поэтому синхронизация нужна перед прогоном).
+Причина — не регрессия продукта, а сравнение координат с плавающей точкой без
+допуска. Новый хелпер `clickStage(x, y)` (строки 18-28) переводит логические
+координаты в `clientX/clientY` через `rect`/`view`, диспатчит настоящий DOM
+`click` на `.stage`, а тест затем требует точного равенства
+`card._path[0][0] === 500 && card._path[0][1] === 500`. Отдельным отладочным
+прогоном подтверждено: тот же клик `(500, 500)` на чистой карте даёт
+`card._path === [[500, 500.00000000000006]]` — снэп к сетке в
+`_resolvePlanDrawPoint`/`_svgPoint` вносит обычную для плавающей арифметики
+погрешность 6·10⁻¹⁴. Второй ассерт (`secondClickCreatesOnlyIndependentSegment`)
+делит то же число на `NORM_W` и сравнивает с `0.5` — получает
+`0.5000000000000001` и тоже падает. Сам продукт при этом работает верно:
+`_draftEndAt` (строки 7621-7636) ищет привязку только в `space.room_drafts`,
+который после `Esc`-finish пуст (черновик уже стал `partitions`), поэтому клик
+по `D` действительно не подхватывает старую точку `C` — просто тест это не
+может подтвердить в текущем виде.
+
+Остальные 8 новых ассертов того же файла (включая три с реальным
+`window keydown` — `escapeFinishesWithoutDeletingSegments`,
+`repeatedEscapeAfterFinishIsNoop`, `rejectedFinishKeepsActiveDraft`, а также
+диалоговые `firstEscapeOnlyCancelsRoomDialog`/`secondEscapeFinishesRestoredDraft`)
+зелёные и доказывают AC1, AC3, AC5, AC6 честно — тест умеет падать: замена
+`card._finishWallChain = () => false` перед `rejectedFinishKeepsActiveDraft`
+подтверждает это на месте.
+
+**Правка:** сравнивать с допуском (как это уже принято для координат в
+проекте, например `Math.hypot(...) <= eps` в `_draftEndAt` этого же файла)
+либо округлять полученную точку перед сравнением, а не требовать `=== 500`
+после реального клика через трансформацию viewBox.
+
+## Что проверено и корректно
+
+- Единственная содержательная правка продукта — одна строка
+  (`src/houseplan-card.ts:2630-2633`): подмена `_undoPoint()` на
+  `_finishWallChain()` в ветке `Escape` при активном `draw` и непустом `_path`.
+  Никакой другой код `_onKey`, `_undoPoint`, `_undoActiveDraftPoint`,
+  `_activateMarkupTool`, приоритет `_roomDialog`/`_physicalDrag` не менялся —
+  риск регрессии сосредоточен ровно в этой ветке, и она разобрана по AC1-AC6
+  выше.
+- `_finishWallChain` при отказе (`MAX_PARTITIONS`) не мутирует `_path` до
+  проверки лимита — active state переживает `false` (AC6), самостоятельно
+  подтверждено чтением строк 6720-6742 и зелёным `rejectedFinishKeepsActiveDraft`.
+- Приоритет `_roomDialog` перед веткой `draw` не нарушен: строка `2623-2627`
+  идёт раньше `2628-2635` и не тронута диффом (AC5).
+- `_resumeDraftBySpace` не сохраняет ссылку на завершённый draft: путь
+  `path.length < 2` (без активного `_activeDraftId`, что гарантировано
+  инвариантом «draft создаётся только при `path.length >= 2`», строка 7758) и
+  путь успешного finish (`delete this._resumeDraftBySpace[this._space]`,
+  строка 6767) оба корректны — прочитано и совпадает с ассертами
+  `!card._resumeDraftBySpace[card._space]`.
+- Все шесть справочных поверхностей из ТЗ §5/AC7 действительно переписаны и
+  построчно сверены с `docs/USER-GUIDE.md`/`docs/USER-GUIDE.ru.md`; старых
+  формулировок «Esc/Ctrl+Z — убрать точку»/«cancels an unfinished path» для
+  контекста Walls не осталось. `markup.hint_points` в `en.json`/`ru.json`
+  совпадает по смыслу с этими описаниями.
+- Оба changelog обновлены в том же коммите `32e8de4f`, что и продукт —
+  `User-Visible: yes` соблюдён.
+- `dist/houseplan-card.js` и `custom_components/houseplan/frontend/houseplan-card.js`
+  идентичны локально пересобранному бандлу (`cmp` совпал).
+- «Одно число — один источник»: этот диф не вводит и не дублирует ни одной
+  видимой пользователю величины (только текст подсказки/руководства); числовых
+  значений (толщина, площадь) правка не касается.
+- Модель геометрии (рёбра, толщина, `layout`, `marker.space`, `open_spans`) не
+  затронута: изменена только точка вызова уже существующей и протестированной
+  `_finishWallChain`, а не сама геометрия/merge/лимиты. `npm test` (включающий
+  инварианты модели на всех моделях проекта) прошёл целиком.
+
+## Чего не проверял
+
+- `npm run golden:verify` — не прогонялся. Диф не меняет рендер геометрии,
+  стилей или слоёв (только текст подсказки и порядок вызова уже
+  протестированной функции), и ни один golden-сценарий не ссылается на
+  `hint_points`/Escape-детач; риск сочтён нерелевантным для этого изменения.
+- `node scripts/model-invariants.mjs --config <…>` отдельно не запускался.
+  Диф не меняет генерацию/мерж геометрии — переиспользует существующий
+  `_finishWallChain`, уже покрытый `smoke_wall_chain_merge.mjs`/
+  `smoke_wall_chain_thickness.mjs` (прогнаны, зелёные) и структурными
+  инвариантами внутри `npm test`.
+- `python -m pytest tests_backend` — не прогонялся, диф не трогает
+  `custom_components/**/*.py`.
+- Полная матрица `demo/smoke_*.mjs` (184 файла) не прогонялась — прогнаны три
+  «прямых совпадения» из `smoke-select`; остальные 181 — предрелизная
+  обязанность (§8), задача не задевает face detection, split, decor, resize,
+  touch, kiosk и т.п.
+- Touch/kiosk-поведение не проверялось руками — правка задевает только
+  desktop-обработчик `Escape`, что подтверждено чтением: единственное
+  добавленное ветвление стоит после `this._tool === 'draw'` в клавиатурном
+  обработчике, тач-жесты (`pointercancel`, pinch, pan) идут через другие ветки
+  того же `_onKey`/`_stagePointer*`, не тронутые диффом.
+- Производительность отдельно не профилировалась — в АС и ТЗ не заявлено
+  влияние на perf-бюджет, а изменение сводится к перенаправлению одного
+  вызова уже существующей O(n) операции.
+
+## Итог
+
+High: 0. Medium: 2, обе в скоупе задачи (обязательный docs-гейт красный;
+собственный новый смок ломается на двух ассертах из-за сравнения
+floating-point координат без допуска, хотя доказываемое им поведение AC2 при
+проверке чтением кода и точечным прогоном подтверждено верным). Без High это
+жёлтый вердикт, возврат автору на правку в этом же issue — отдельный issue не
+заводится (обе находки в скоупе, §2.7/#202).