From 33dcd1ce6225f8983687e1a08ea543831f2c338a Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Tue, 25 Aug 2026 14:09:15 +0000 Subject: [PATCH] docs: review document for #304 Issue: #304 User-Visible: no --- docs/reviews/CODE-REVIEW-304-r1.md | 272 +++++++++++++++++++++++++++++ 1 file changed, 272 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-304-r1.md diff --git a/docs/reviews/CODE-REVIEW-304-r1.md b/docs/reviews/CODE-REVIEW-304-r1.md new file mode 100644 index 00000000..8ebca87f --- /dev/null +++ b/docs/reviews/CODE-REVIEW-304-r1.md @@ -0,0 +1,272 @@ +# CODE-REVIEW-304-r1 + +**Issue:** [#304 — паритет базовых осей и узлов между инструментами Plan editor](https://github.com/Matysh/houseplan-card/issues/304) +**Ветка:** `issue/304-plan-axis-parity`, HEAD `7310ce0134ca8e5b1c32e6816a2e70625ea158e2` +**Диапазон:** `origin/dev...HEAD` (2 коммита: `a2b4d32b` — код, `7310ce01` — скриншоты документации) +**Трек:** `small` (лёгкий), ТЗ в теле issue, ревью ТЗ — комментарий (зелёный, заход r1, 0/2 циклов, 2026-08-25T13:06:27Z) +**Заход:** r1 (первый код-ревью для этого issue; предыдущих раундов код-ревью нет, раздел «Унаследовано из r» неприменим) +**Вердикт:** жёлтый · High: 1 · Medium: 0 + +## Скоуп + +Контракт (тело issue): статический слой архитектурных осей/узлов (`plan-snap-overlay`) +должен рендериться во всех 10 `MarkupTool`, а не только в «Стены» (`draw`); transient +snap/hover остаются эксклюзивными для «Стены»; View/Device editor/Background editor слой +не получают. + +Диапазон коснулся: +- `src/houseplan-card.ts` — единственная продуктовая правка (класс A), 5 добавленных / 2 + удалённые строки; +- `demo/smoke_plan_snap_overlay.mjs`, `demo/golden/harness.mjs`, `test/plan-snap-overlay.test.mjs` — тесты (класс B); +- `docs/CHANGELOG.md`, `docs/CHANGELOG.ru.md`, `docs/STATUS.md`, `docs/USER-GUIDE.md`, + `docs/USER-GUIDE.ru.md`, `docs/images/**`, `docs/images/screenshots.json` — документация (класс C); +- `dist/houseplan-card.js`, `custom_components/houseplan/frontend/houseplan-card.js` — сгенерированное (класс D), синхронно с `src`. + +`demo/golden/matrix.mjs` и `src/plan-snap-overlay.ts` **не тронуты** — геометрия и матрица +сцен не менялись, что соответствует заявленному «не-скоупу». + +## Сама правка + +```ts +// было +${this._markup && this._tool === 'draw' ? svg` + ${this._renderPlanSnapOverlay()}` : nothing} +… +private _renderPlanSnapOverlay(): TemplateResult { + if (!this._markup || this._tool !== 'draw') { return svg``; } + +// стало +${this._markup ? svg` + ${this._renderPlanSnapOverlay()}` : nothing} +… +private _renderPlanSnapOverlay(): TemplateResult { + if (!this._markup) { return svg``; } +``` + +Гейт `_tool === 'draw'` снят в двух местах; `_activePlanSnapCandidate`/`_activePlanSnapConflicts` +(строки 6996–7010, не изменены) по-прежнему возвращают `null`/`[]` вне `draw` — transient-маркер +и conflict-подсветка остаются эксклюзивными «Стенам», как требует контракт п.4. `_markup` +(строка 1524, не изменена) — это `_mode === 'plan'`, поэтому View/`devices`/`decor` не получают +слой без дополнительных условий (контракт п.6). + +## Как проверялось + +### Дешёвые гейты — прогнаны все + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | зелёный, exit 0 | +| Unit | `npm test` | 1299 тестов, 1298 passed, 1 skipped (pre-existing `#281` private-fixture skip, не связан с #304), 0 failed | +| Build + sync | `npm run build && npm run bundle:sync` | зелёный; `dist/houseplan-card.js`, `custom_components/houseplan/frontend/houseplan-card.js`, `demo/srv/assets/houseplan-card.js` byte-identical (`cmp`) | +| Docs fingerprint | `node scripts/check-docs.mjs` | «Documentation checks passed (7 files, 10 external links)» | +| Выбор смоков | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | 7 прямых совпадений по символу `_markup`; символ `_tool` инструмент не посчитал широким | +| Model invariants | — | не запускал: диф не трогает рёбра комнат, `layout`, `marker.space`, `open_spans`, записи толщины или `plan-snap-overlay.ts` — только условие рендера. Проверено чтением | + +### Дисциплина «тест умеет падать» — проверено активно, не на слово + +Временно откатил `src/houseplan-card.ts` к состоянию `origin/dev` (`git apply -R`), пересобрал +бандл (`npm run bundle:sync`) и повторно прогнал: + +- `node demo/smoke_plan_snap_overlay.mjs` → падает необработанным исключением + (`Cannot read properties of null (reading 'querySelectorAll')`) на проверке `wallthick` — + без фикса `overlay()` для этого инструмента `null`, ассерт паритета невыполним; +- `node demo/golden/run.mjs --mode=capture --scenario=hidden-wall-diagnostics-plan-light` → + статус `error` (новая проверка паритета внутри `prepareGoldenScenario` бросает исключение до + снятия скриншота). + +Затем восстановил фикс (`git apply`), пересобрал бандл — дерево вернулось к состоянию коммита +(`git status` пусто), оба прогона снова зелёные. Тест и golden-инвариант доказанно способны падать. + +### Смоки — прямые совпадения плюс релевантные по AC6, прогнаны лично + +``` +node demo/smoke_edit_walk.mjs OK +node demo/smoke_editor_gestures.mjs OK +node demo/smoke_merge_split.mjs OK +node demo/smoke_optimize_coincident_partition.mjs OK +node demo/smoke_resize_audit_1550.mjs OK +node demo/smoke_room_resize.mjs OK +node demo/smoke_split_nonsnap.mjs OK +node demo/smoke_plan_snap_overlay.mjs OK (все 43 подпроверки true) +node demo/smoke_wall_thickness.mjs OK +node demo/smoke_opening_preview.mjs OK +node demo/smoke_partition_openings.mjs OK +node demo/smoke_resize_wall_thickness.mjs OK +node demo/smoke_unified_wall_tool.mjs OK +node demo/smoke_wallthick_hover_width.mjs OK +``` + +Полный набор `demo/smoke_*.mjs` (188 файлов) не прогонялся — задача не задевает всё, только +рендер-слой Plan editor; полный набор остаётся предрелизным гейтом (§8). + +### Golden — локально плюс канонический Linux CI-артефакт с branch'а автора + +Локально (native Windows, диагностически, per `AGENTS.md` «Известное окружение»): +`node demo/golden/run.mjs --mode=capture --scenario=hidden-wall-diagnostics-plan-light|dark` — +оба **passed** (пиксельное совпадение с текущим эталоном, не только «семантика»). + +Для полной картины поднял фактический прогон `Validate` на этой же ветке этим же автором +(GitHub Actions, `gh run view/api`): + +- commit `a2b4d32b` (сама правка, до докс-коммита), run `32854408646`: + job `golden` → **failure**. Полный список сцен смотрел через логи джобы; 110 сцен `passed`, + **5 сцен `different`**: `safe-resize-handles-clamp-light`, `safe-resize-handles-clamp-dark`, + `opening-placement-door-thick-wall-dark`, `opening-placement-passage-thick-wall-dark`, + `opening-placement-passage-thick-wall-light`. Смок `smoke (1)` и `performance_smoke` — + `cancelled` (не успели: следующий пуш отменил прогон), `docs` — `failure` (ожидаемо, устаревший + отпечаток скриншотов, закрыто следующим коммитом). +- те же 5 сцен на `dev` (run `32848267582`, job `golden`, commit `2143e888`) — все **passed**. + Разница вызвана этой веткой, не флуктуацией CI. +- скачал артефакт `golden-images` (`gh api .../artifacts/9565735845/zip`) и посмотрел + `diff/safe-resize-handles-clamp-light.png`, `diff/opening-placement-door-thick-wall-dark.png`, + `diff/opening-placement-passage-thick-wall-light.png` — во всех трёх новый статический слой + осей/узлов (тот самый `plan-snap-line`/`plan-snap-node`) закономерно появляется поверх сцены, + потому что `prepareGoldenScenario` для этих сценариев (`safeResizePreview` → `card._tool = + 'resize'`, `openingPreview` → `card._activateOpeningPlacement(type)` переводит в `_tool = + 'opening'`) не возвращает `_tool` в `draw` перед снимком — в отличие от нового кода для + `scenario.hiddenWallDiagnostics`, который явно делает `card._tool = 'draw'` перед финальным + кадром (`demo/golden/harness.mjs:709-711`). См. finding ниже. + +## Разбор по AC + +- **AC1 (паритет контрольного узла, smoke+golden).** Смок: новая fixture «room + 2 coincident + partitions» даёт 6/6 узлов и линий в `draw` и `wallthick` (`thicknessShowsCompleteSixNodeFixture`). + Golden: существующие сцены `hidden-wall-diagnostics-plan-{light,dark}` (комната + coincident + partition + скрытая сохранённая цепочка) проходят пиксельно без изменений эталона, и новая + проверка внутри `prepareGoldenScenario` (строки 675-711) явно перебирает все 10 инструментов и + бросает исключение при расхождении DOM до снятия скриншота. **Выполнено.** +- **AC2 (все инструменты, smoke).** `allPlanToolsShareStaticAxesAndNodes` сравнивает нормализованный + DOM-снимок (`data-key`, координаты) по всем 10 `MarkupTool`; `allPlanToolOverlaysPaintAboveWallBodies` + подтверждает порядок относительно `.wallbodies`. **Выполнено.** +- **AC3 (дедупликация, unit+smoke).** Новый unit-тест `test/plan-snap-overlay.test.mjs` фиксирует + 6 сегментов/6 узлов для той же топологии на уровне `buildPlanSnapGeometry` (сама функция не + менялась — диф `src/plan-snap-overlay.ts` пуст, что и ожидалось: контракт дедупликации не задет). + Утверждение «hidden diagnostic не создаёт визуального двойника» — унаследованное поведение + нетронутой `buildHiddenWallDiagnosticGeometry` (уже сосуществовала с полным overlay в `draw` до + фикса); проверено чтением, не новым тестом. **Выполнено.** +- **AC4 (интерактивность, smoke).** `allPlanToolOverlaysStayPointerTransparent` проверяет + `pointer-events: none` и оба вычисляемых стиля во всех 10 инструментах. Вторая часть AC4 + («применение толщины меняет только выбранный интервал») не тестировалась заново на новой + fixture — код применения толщины (`_wallDialog` и связанный путь) диффом не тронут, оверлей был + `pointer-events: none` и до фикса; проверено чтением. `smoke_wall_thickness.mjs` (несвязанная + fixture) остаётся зелёным. **Выполнено**, вторая половина — «проверено чтением, не исполнением». +- **AC5 (границы режима, smoke).** `nonPlanModesHaveNoOverlay` проверяет `devices`, `decor`, `view` + без слоя; `_markup` (не изменён) гарантирует то же на уровне кода. Отсутствие промежуточного + кадра без осей при переключении инструмента (контракт п.7) — структурно невозможно: видимость + зависит только от `_markup`, не от `_tool`; `_tool` не участвует в тайминге рендера. Проверено + чтением. **Выполнено.** +- **AC6 (доступность и регрессии, smoke+golden).** `forcedColorsStayReadable` зелёный (существующая + проверка, поведение не изменено). Целевые смоки draw/openings/resize (см. список выше) зелёные. + **Но** golden-доказательство «без изменения поведения» для двух смежных сценариев неполно — + см. finding H1. **Частично выполнено.** +- **AC7 (данные и производительность, unit + ревью кода).** Диф не трогает `_planSnapGeometrySnapshot`, + кеш или что-либо на pointermove; конфигурация не мутируется, сохранение не вызывается при смене + инструмента (не добавлено ни одного нового вызова записи). Проверено чтением: единственное + изменение — булево условие рендера. **Выполнено.** + +## Находки + +### H1 (High) — два golden-сценария неродственных фич стали «different» на канонической Linux CI и не получили новый эталон + +**Файлы:** `demo/golden/harness.mjs` (`safeResizePreview`, `openingPreview` — строки 946-1033), +`demo/golden/matrix.mjs` (сценарии `safe-resize-handles-clamp-{light,dark}`, +`opening-placement-{door,passage}-thick-wall-{dark,light}`), `demo/golden/baselines/*.png` (не +обновлены). + +**Воспроизведение:** GitHub Actions, репозиторий `Matysh/houseplan-card`, run `32854408646` +(`Validate` на commit `a2b4d32b`, та же ветка), job `golden` → +https://github.com/Matysh/houseplan-card/actions/runs/32854408646/job/97823146408 — статус +`failure`; 5 из 115 сцен: `different`. Те же 5 сцен на `dev` (run `32848267582`, job `golden`, +commit `2143e888`) — все `passed`, то есть расхождение вызвано именно этой веткой, не шумом CI. +Скачанные diff-изображения (`golden-images` artefact, `9565735845`) показывают новый слой +`plan-snap-line`/`plan-snap-node`, появившийся в сценах `resize` и `opening`, где раньше рендерился +пустой ``. + +**Причина:** контракт issue (п.1) прямо требует показывать статический слой во всех 10 +инструментах, включая `resize` и `opening` — то есть новое появление осей/узлов в этих golden-сценах +корректно и ожидаемо. Но `prepareGoldenScenario` для `scenario.hiddenWallDiagnostics` явно +возвращает `card._tool = 'draw'` перед финальным кадром (`demo/golden/harness.mjs:709-711`), +а `scenario.safeResizePreview` (переводит в `resize`) и `scenario.openingPreview` (переводит в +`opening`) — не возвращают. Пять существующих, не относящихся к #304 сцен (`safe-resize-handles-clamp-*`, +`opening-placement-*-thick-wall-*`) снимаются в этих инструментах и теперь легитимно отличаются +от принятого эталона, но новый эталон не снят и не принят: `git diff origin/dev...HEAD -- +demo/golden/baselines/` пуст. + +**Почему это блокирует, а не Low:** AC6 требует «без изменения поведения» для этих же +инструментов, а issue прямо перечисляет `visual/golden доказательство` как обязательный +release-артефакт (раздел «Release-артефакты»). Сейчас доказательство — отрицательное: канонический +Linux-гейт красный для пяти сцен, и это не отражено ни в одном коммите, ни в хендофф-комментарии +автора («golden:verify на native Windows — diagnostic only… Канонический полный Linux golden +остаётся обязательным pre-beta gate»), хотя автор сам инициировал этот прогон и мог его увидеть. +Без действия эти пять эталонов останутся неразрешённо «different» на предрелizном гейте — дефект, +который дешевле поймать сейчас (issue #237 — тот же класс пропуска, только для `check-docs`). +Соответствует условию жёлтого вердикта из процесса: «изменение… ухудшает смежный [сценарий]» +(§7.2/AGENTS.md) — с точки зрения непринятого эталона резинка Resize и предпросмотр проёма в +толстой стене визуально изменились без прошедшего ревью. + +**Что нужно для исправления (в скоупе этой же задачи, не отдельный issue — причина в диффе этой +ветки):** +1. на Linux (CI, не native Windows) снять `npm run build && npm run bundle:sync && npm run + golden:capture` для матрицы; проверить, что «different» ограничивается именно этими 5 сценами + (или объяснить, если появились новые); +2. принять новые эталоны `npm run golden:accept -- --reviewed --from=<распакованный артефакт + Linux CI>` — **важно:** текущий `Validate` на самом HEAD (`7310ce0`, run `32855061198`) сам + пропустил job `golden` (`changes` классифицирует диф **относительно предыдущего пуша**, а не + `origin/dev`, и коммит `7310ce0` — только докс/скриншоты); чтобы получить свежий полный + Linux-артефакт для приёмки, нужен новый пуш/ре-триггер `Validate` на этой ветке; +3. отразить пересъёмку в хендофф-комментарии (команда + результат, не «verified»). + +### Low (снято без правки, не блокирует) + +- AC1 фиксирует «шесть узлов» через частный unit/smoke fixture, а не через golden-сцену с + буквально такой же топологией (golden использует смежный, но не идентичный `golden-coincident-partition` + фикстур). Разумно — тот же класс регресса (совмещённые/скрытые оси) реально покрыт, дублировать + фикстуру в третьем месте не нужно. +- Комментарий-документация в шаблоне (`${''/* … */}`) — существующий в файле стиль (см. строки + 17560, 17568), новый блок ему соответствует, замечаний нет. + +## Что проверено и корректно + +- Гейтинг сведён к одному булеву условию, transient state (`_activePlanSnapCandidate`, + `_activePlanSnapConflicts`) остаётся эксклюзивным «Стенам» — контракт п.4 не нарушен. +- Порядок слоёв не менялся (условие снято на уже существующем месте в шаблоне) — риск «render + order перекрыл preview другого инструмента», названный в issue, не реализовался нигде, кроме + golden-сцен из H1, где это ожидаемо и просто не задокументировано эталоном. +- View/Device editor/Background editor не получают слой ни по коду, ни по смоку. +- i18n не тронут (нет диффа в `src/i18n/*`, `custom_components/**/translations/*`), миграции и + compatibility-поля не нужны — конфигурация и `plan-snap-overlay.ts` не менялись. +- Трейлеры: оба коммита несут `Issue: #304`; `a2b4d32b` — `User-Visible: yes` с правками в обоих + changelog в том же коммите; `7310ce01` — `User-Visible: no`, докс-только, скриншоты снял workflow + `Docs screenshots` (run `32854424829`, конклюзия `success`, SHA совпадает с `a2b4d32b`) — процесс + скриншотов (§8 «только джобой Docs screenshots») соблюдён. +- `node scripts/process-gate.mjs --range=origin/dev..HEAD` — «гейт пройден», единственное `WARN` + (нет `docs/specs/304-*.md`) — ожидаемо для `small`-трека. +- Один источник числа: диф не вводит новую видимую пользователю величину (только топологию + линий/узлов), правило «одно число — один источник» неприменимо. + +## Чего не проверял + +- Полный `demo/smoke_*.mjs` (188 файлов) и полный `npm run golden:verify` локально — не запускал, + задача не задевает все поверхности; вместо этого использовал целевую выборку + (`smoke-select.mjs`) плюс фактический канонический Linux-прогон автора для golden (сильнее + локального прогона на native Windows). +- `python -m pytest tests_backend` — не запускал, `custom_components/**/*.py` не тронут. +- Performance-профили и large-house benchmark — не запускал: `_planSnapGeometrySnapshot`/резолвер + не тронуты, влияние на pointermove отсутствует по чтению кода, в AC не названо. +- `node scripts/model-invariants.mjs` — не запускал: диф не касается рёбер комнат, `layout`, + `marker.space`, `open_spans`, записей толщины. +- Визуальную читаемость двух новых/скорректированных строк `docs/USER-GUIDE.md`/`.ru.md` смотрел + только текстом; PNG-скриншоты (`docs/images/*.png`) не сверял пиксельно — доверился принятому + workflow `Docs screenshots` и `check-docs.mjs`. + +## Итог + +Продуктовая правка минимальна, соответствует контракту, транзитное состояние и границы режимов не +нарушены — подтверждено и чтением, и смоками, включая доказанную способность нового смока и нового +golden-инварианта падать. Но обязательное по контракту и по разделу «Release-артефакты» golden- +доказательство неполно: два неродственных, уже существующих сценария (`safe-resize-handles-clamp-*`, +`opening-placement-*-thick-wall-*`) корректно и ожидаемо изменили внешний вид под тем же самым +кодом, но не получили пересъёмку/приёмку эталона, и это осталось незамеченным/незадокументированным +в хендоффе при том, что канонический Linux CI уже показал `golden: failure` на этой ветке. +Возврат автору для пересъёмки и приёмки этих пяти эталонов на Linux — фикс укладывается в эту же +задачу (причина в её собственном диффе), отдельный issue не нужен.