From 51fde854e3a2b8dd18fc08e096893cc57ce71bca Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sat, 22 Aug 2026 12:24:25 +0000 Subject: [PATCH] docs: review document for #224 Issue: #224 User-Visible: no --- docs/reviews/CODE-REVIEW-224-r1.md | 233 +++++++++++++++++++++++++++++ 1 file changed, 233 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-224-r1.md diff --git a/docs/reviews/CODE-REVIEW-224-r1.md b/docs/reviews/CODE-REVIEW-224-r1.md new file mode 100644 index 00000000..f1999de1 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-224-r1.md @@ -0,0 +1,233 @@ +# CODE-REVIEW-224-r1 + +- Issue: [#224](https://github.com/Matysh/houseplan-card/issues/224) — канонические координаты на каждой записи +- ТЗ: `docs/specs/224-config-coordinate-canonicalization.md`, ревью ТЗ зелёное (`docs/reviews/SPEC-REVIEW-224-r1.md`) +- Диапазон: `origin/dev...HEAD`, HEAD = `4a798e3e13275c0820aec2db1e63d1fae31a41f3` +- Заход: r1 (первый код-ревью этой задачи, дельта не применяется — разбор полный) +- Вердикт: **красный** · High: 1 · Medium: 2 (обе в скоупе) · Low: 1 + +## Скоуп + +28 файлов, +2050/-196: общий Python/TypeScript контракт квантования +(`custom_components/houseplan/coordinate_canonicalization.py`, +`src/coordinate-canonicalization.ts`), подключение к `CONFIG_SCHEMA` / +`LAYOUT_SCHEMA` / `POS_SCHEMA` и к storage barrier (`store.py`), no-op ветки в +`websocket_api.py` (`config/set`, `layout/set`, `layout/update`), фронтенд +(`_writeConfig`, `_persistLayout`), общая fixture, backend/frontend тесты, +четыре мутанта из issue, документация (CANVAS/CONFIG-COMPATIBILITY/USER-GUIDE/ +CHANGELOG×2/TESTING/STATUS). + +## Как проверялось + +| Гейт | Результат | +|---|---| +| `npx tsc --noEmit` | зелёный | +| `npm test` | 1061 тестов, 0 упало | +| `npm run build` + сверка `dist` / `custom_components/.../frontend` / `demo/srv/assets` | все три копии побайтово идентичны | +| `node scripts/check-docs.mjs` (обязателен: diff трогает `src/**`) | **красный** — `ERROR screenshot source fingerprint is stale; run npm run build && node demo/docs/capture.mjs` (см. Finding M1) | +| `python -m pytest tests_backend -q` (обязателен: diff трогает `custom_components/**/*.py`) | 346 passed, 1 error — ошибка та же в изоляции на `tests_backend/test_ha_upload.py::test_upload_ok` и воспроизводится на голом запуске этого файла: teardown-квирк `pytest-homeassistant-custom-component` (`_run_safe_shutdown_loop` thread-name assert), не связан с диффом. Целевые файлы (`test_coordinate_canonicalization.py`, `test_ha_websocket.py`, `test_ha_import_export.py`) — 159 passed, тот же посторонний teardown-error один раз | +| `npm run golden:verify` (по необходимости: diff меняет геометрию/рендер) | **красный** — 23 сцены `different` из ~82 (см. Finding H1) | +| Мутанты из issue (`node scripts/mutation-gate.mjs --id=<...>`) | все 4 названных в issue id пойманы 1 из 1: `schema-quantization-removed`, `frontend-writes-raw-coords`, `quantization-hits-allowlist`, `import-path-bypasses-schema` | +| Смоки по grep (`_writeConfig`/`canonicaliz`) | `node demo/smoke_config_writer.mjs` — OK; `node demo/smoke_optimize_coordinate_canonicalization.mjs` — OK (мокает `callWS`, не проходит через реальный backend, поэтому не свидетельствует ни за, ни против canonicalization) | +| Полный `mutation-gate.mjs` (сверх обязательного, для очистки совести) | один посторонний бейзлайн-фейл вне #224 (`demo/smoke_hide_room_names.mjs`, `trueRestoresExistingCard`), файл диффом не тронут — вне скоупа этой задачи, не блокирует | + +Не проверялось: `demo/smoke_*` полным набором (163 штук — задача не задевает всё +поле); performance-профили (не названы в AC и не тронуты чувствительные пути, +спецификация явно говорит "нет отдельного perf-артефакта"). + +## Находки + +### H1 — golden не остаётся прежним, вопреки AC12 и §2/§15 ТЗ (High, в скоупе) + +`npm run golden:verify` на этой ветке даёт **23 `different` сцены** из ~82. +Сравнение с `origin/dev` в том же окружении (тот же Chromium/Linux, тот же +прогон подряд дважды — набор стабилен, не флейк) показывает, что **3** из них +(`opening-placement-door-thick-wall-*`) уже отличаются на `dev` и к #224 +отношения не имеют; но **20** возникают именно на этой ветке: + +``` +geometry-view-dark-fit, geometry-view-light-fit, +day-cycle-dawn/day/dusk/night-dark (4), +geometry-plan-editor-dark, plan-snap-endpoint-light, plan-snap-line-gaps-dark, +geometry-devices-editor-dark, geometry-decor-editor-dark, +tray-wide-selection-en, tray-wide-tool-ru, tray-medium-group-en, +tray-medium-selection-ru, tray-narrow-tool-ru, +geometry-diagonal-45-opening-dark, hover-nested-room-dark, +backup-full-preview-desktop-en, backup-plan-only-export-desktop-en +``` + +Отличия не косметические: `maxObservedDelta` до 210 (порог сцены — 10), +`diffRatio` до 0.7% (порог — 0.05–0.08%), т.е. `golden-report.json` показывает +реальный, не пороговый шум. Пиксельный разбор `geometry-view-dark-fit` +(`artifacts/golden/actual` против `demo/golden/baselines`) в районе +x=467–486, y=44–375 показывает не микросдвиг канта, а качественно другой +результат: там, где baseline рисует широкое пятно (иконка/бейдж устройства, +цвета 92–230), actual даёт двухпиксельную линию — похоже на элемент, +который перестал полноценно рендериться, а не на ULP-дребезг стены. + +Это прямо противоречит: +- AC12 «Visual golden не меняется»; +- §2 ТЗ «Внешний вид и точность размещения не меняются»; +- §15 ТЗ «Новых ... golden baseline ... нет». + +Диагностика, которую успел провести: `canonicalizeConfigGeometry()`, +прогнанный оффлайн на фикстуре `golden-geometry` +(`demo/fixtures/visual-matrix.mjs`), возвращает **побитово идентичный** +объект — то есть сам алгоритм квантования не трогает эти конкретные числа. +Мок golden-харнесса (`demo/golden/harness.mjs`) также не проходит через +`_writeConfig`/`_persistLayout` (`config/get`/`layout/get` отдают +`structuredClone(fixture...)` напрямую, `config/set` в моке — заглушка +`{ ok: true }`), поэтому новый код квантования в View-сценах в принципе не +выполняется. Значит причина не в «шум стал виден», а в каком-то другом +побочном эффекте этого диффа на рендер (кандидат, который я не успел +исключить: лишний авто-инкремент `_cfgEpoch` при каждой синхронной +переприсвоении `_serverCfg`, см. Low L1, — но это не объясняет чисто +View-сцены без единого `_writeConfig`). Причина остаётся на автора: ревью +только фиксирует и воспроизводит факт регресса, не диагностирует его до +конца. + +**Воспроизведение:** `npm run build && cp dist/houseplan-card.js +demo/srv/assets/houseplan-card.js && npm run golden:verify`, сравнить с тем +же прогоном на `origin/dev`. + +### M1 — `check-docs.mjs` красный: отпечаток скриншотов не обновлён (Medium, в скоупе) + +`node scripts/check-docs.mjs` падает: `ERROR screenshot source fingerprint is +stale; run npm run build && node demo/docs/capture.mjs`. Diff трогает +`src/houseplan-card.ts` и добавляет `src/coordinate-canonicalization.ts`, а +`docs/images/screenshots.json` в этом диффе не менялся (`git diff +origin/dev...HEAD --stat -- docs/images` пуст). Это ровно тот класс +регрессии, который PROCESS.md §8 называет неусловным: «любая правка +фронтенда делает [отпечаток] устаревшим», и который уже стоил `dev` +красного job `docs` на #230 и #234 (issue #237). Правка механическая: +`npm run build && node demo/docs/capture.mjs`, закоммитить обновлённый +`docs/images/screenshots.json` (и сами скриншоты, если изменились) в этой же +задаче. + +### M2 — AC6 заявляет доказательство, которого нет (Medium, в скоупе) + +Спецификация (§12, AC6) называет способ доказательства: «Queue/debounce unit +с controlled promises». В диффе такого теста нет — единственная фронтенд- +проверка соответствующего кода это +`test('frontend write paths adopt canonical candidates before persistence +(#224)', ...)` в `test/coordinate-canonicalization.test.mjs`, которая +сверяет **текст исходника регулярным выражением**, а не поведение очереди +записи (не создаёт конкурентных промисов, не проверяет, что правка, +сделанная во время `await callWS`, не теряется). + +По коду поведение действительно верное — **проверено чтением, не +исполнением**: `_writeConfig()` (`src/houseplan-card.ts:6948`) присваивает +`this._serverCfg = candidate` синхронно, до `await this.hass.callWS(...)`, и +каждое звено `_writeChain` читает `this._serverCfg` в момент своего +выполнения (комментарий над полем прямо описывает этот контракт), поэтому +правка, сделанная во время отправки, действительно войдёт в следующее звено +очереди. Функционально AC6 похоже выполнен. Но раз обещанного тестом +доказательства нет, а регресс-тест на конкурентную запись мог бы поймать +именно такие ошибки (в т.ч. в паре с H1, где причина рендер-регресса пока не +установлена), это заявленный, но не выполненный пункт AC — правится тестом +в этой же задаче либо явной пометкой в ТЗ/хендоффе, что доказательство +заменено на «проверено чтением». + +### L1 — двойной инкремент `_cfgEpoch` в `_writeConfig` (Low) + +`src/houseplan-card.ts:6955-6958`: + +```ts +const candidate = canonicalizeConfigGeometry(this._serverCfg); +const candidateFingerprint = contentFingerprint(candidate); +if (candidateFingerprint !== contentFingerprint(this._serverCfg)) this._cfgEpoch++; +this._serverCfg = candidate; +``` + +`_serverCfg` зарегистрирован реактивным (`static properties = { _serverCfg: +{ state: true }, ... }`), и `willUpdate()` уже безусловно делает +`this._cfgEpoch++` при `changed.has('_serverCfg')` +(`src/houseplan-card.ts:3589`). Поскольку `canonicalizeConfigGeometry` +всегда возвращает свежий клон (новую ссылку) — даже когда контент не +изменился, — присваивание `this._serverCfg = candidate` **само по себе** +уже гарантирует один инкремент эпохи на следующем рендере. Ручной инкремент +в строке 6957 срабатывает только когда контент реально изменился, то есть +именно тогда сверху добавляется второй, лишний инкремент. Эффект не +наблюдаем пользователем (эпоха используется только для сравнения на +равенство в кэшах), но это мёртвая/дублирующая логика — можно снять решением +ревьюера с записью здесь, без правки. + +## Что проверено и корректно + +- Контракт квантования (`canonicalize_number`/`canonicalizeNumber`): + 9 знаков, round-half-away-from-zero, нормализация `-0`, побитовое + совпадение Python/TypeScript на общей fixture (AC1) — тест умеет падать + (мутант `quantization-hits-allowlist` ловится). +- Allowlist (§6.2) и negative contract (§6.3) реализованы идентично на обеих + сторонах, unknown-поля не материализуются, вход не мутируется (проверено + тестами `test_coordinate_canonicalization.py` и + `coordinate-canonicalization.test.mjs`, идемпотентность подтверждена). +- Backend schema barrier (`CONFIG_SCHEMA`/`LAYOUT_SCHEMA`/`POS_SCHEMA`) и + общий storage barrier (`async_save_config_state`/`layout_store_payload`) + канонизируют независимо от источника записи — включая внутренних writer-ов + (import apply/rollback, Optimize backup/pending, geometry repair backup, + Optimize Undo) — подтверждено чтением всех точек вызова в + `websocket_api.py` и тестом `test_storage_helpers_are_the_final_canonical_barrier` + (AC5, AC7, AC8). +- No-op контракт для `config/set`/`layout/set`/`layout/update`: сравнение + канонического live-кандидата со стором, `ok:true` с прежним `rev`, store + не пишется, event не публикуется, Optimize-снимок не теряется, CAS + проверяется ДО сравнения содержимого (устаревший `expected_rev` всё ещё + конфликт) — воспроизведено и подтверждено тестом + `test_canonical_rewrites_are_noops_without_events_or_undo_loss`, тест умеет + падать (мутация: убрать `if` — тест сразу поймает лишнюю ревизию/событие). + Отдельно проверено, что no-op в `ws_config_set` не вызывает file collection + (`collect_plans`/`collect_attachments`) — только best-effort обновление + Repairs (`async_check_plan_files`, не удаляет файлы), что согласуется с + инвариантом SCOPE.md «никогда не удалять файл по догадке» (AC4). +- Регресс #218 (шесть комнат, общие ULP-вершины): новый тест в + `test/physical-geometry.test.mjs` подтверждает непустой union и непустой + Glow-клип после канонизации, а исходный editor-candidate не мутируется + (AC9); тест умеет падать (тот же union/clip раньше требовал ручной + round-обёртки, без канонизации фиктура давала пустое пересечение — это и + есть исходная жалоба #218). +- Диагональные/не-решёточные координаты не «прилипают» к сетке — числовой + допуск 5e-10 подтверждён юнитами на обеих сторонах (AC10). +- Denylist (температуры/цвета/калибровка/`view_box`/presentation) не + затрагивается — негативная fixture плюс мутант + `quantization-hits-allowlist` (AC11). +- Импорт (`import_export.py:582,592`) идёт через те же `CONFIG_SCHEMA`/ + `LAYOUT_SCHEMA`, что и обычная запись — новый backend-тест плюс мутант + `import-path-bypasses-schema` (AC7). +- Формулировка Undo из #223 обновлена в `docs/TESTING.md` и + `docs/USER-GUIDE.ru.md` в соответствии с §9 ТЗ («canonical representation» + вместо «noisy bits»); backend-тест + `test_optimize_undo_restores_geometry_but_not_legacy_noisy_bits` + подтверждает, что Undo восстанавливает каноническое значение, читая + напрямую из pre-#224 (noisy) снимка стора — тест умеет падать (AC8). +- Трейлеры коммита: `Issue: #224`, `User-Visible: yes`, оба changelog + (`docs/CHANGELOG.md`, `docs/CHANGELOG.ru.md`) правлены в том же коммите + `4a798e3`. Терминология в `docs/USER-GUIDE.ru.md` («не привязка к сетке», + «Оптимизировать планы») согласована с разделом 19 того же документа. +- Три копии продакшн-бандла синхронны. + +## Чего не проверял + +- Полный набор `demo/smoke_*.mjs` (163 шт.) — задача не касается всех + поверхностей; прогнаны только те, что совпали с изменёнными именами/полями + по grep (`smoke_config_writer`, `smoke_optimize_coordinate_canonicalization`). +- Performance-профили — не названы в AC, спецификация явно отказывается от + отдельного perf-артефакта, изменённый код — один линейный обход allowlist. +- Полный `mutation-gate.mjs` сверх четырёх мутантов issue не является + обязательным гейтом ревью; прогнан частично из любопытства, найденный + посторонний фейл (`smoke_hide_room_names.mjs`) вне скоупа #224 и не влияет + на вердикт. +- WSL/Windows-специфичные смоки — ревью идёт в Linux CI-подобном окружении, + что ближе к канону, чем локальная машина автора. + +## Итог + +High-находка (H1) блокирует: заявленное в AC12 и §2/§15 ТЗ «внешний вид не +меняется» эмпирически неверно на этой ветке — 20 golden-сцен реально +отличаются от baseline, включая то, что похоже на пропавший визуальный +элемент, а не на дребезг последнего бита. Причину не установил до конца +(сам алгоритм канонизации — доказанно no-op на этих данных, а +задействованные write-пути не выполняются в View-режиме харнесса), поэтому +прошу автора локализовать источник рендер-регресса, а не просто принять +новые baseline. Вместе с двумя Medium (гейт `docs` и недостающий тест AC6) +возврат в «В разработке».