From 43f91a214b3a7d44a79351e03ecd3d5aabcd2d7b Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Wed, 26 Aug 2026 07:47:10 +0000 Subject: [PATCH] docs: review document for #295 Issue: #295 User-Visible: no --- docs/reviews/CODE-REVIEW-295-r1.md | 297 +++++++++++++++++++++++++++++ 1 file changed, 297 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-295-r1.md diff --git a/docs/reviews/CODE-REVIEW-295-r1.md b/docs/reviews/CODE-REVIEW-295-r1.md new file mode 100644 index 00000000..e13d0f72 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-295-r1.md @@ -0,0 +1,297 @@ +# CODE-REVIEW #295 — r1 + +Issue: https://github.com/Matysh/houseplan-card/issues/295 +Ветка: `issue/295-preflight-diagnostics`, ревьюируемый HEAD: `822e6223f74ae0df993e1f9e9e702faa67fd12ba` (после ребейза на `dev`, CI run [32942935178](https://github.com/Matysh/houseplan-card/actions/runs/32942935178) — зелёный). +Спецификация: `docs/specs/295-preflight-diagnostics.md`, ревизия 2 (принята SPEC-REVIEW-295-r2, зелёный). +Заход: r1 (первый цикл этапа `code`) — разбор полный, разделы «Закрытие r» / «Унаследовано» не применимы. + +## Скоуп + +Диалог «Оптимизировать» при отказе preflight (#199) называет причину по каждому +пространству (7 значений `OptimizeGeometryFailureReason` → i18n), даёт кнопку +«Скопировать диагностику» (JSON с `origin: runtime`, версией карточки, +отпечатками, классом исключения; инлайн-фолбэк при недоступном clipboard), +пишет структурированную запись в dev-лог (дедуп по fingerprint), и показывает +условный совет «обновите» только при реальном расхождении версий карточки и +интеграции (новое поле `integration_version` в `houseplan/config/get`). +Совпадает с ревизией 2 спецификации, скоуп не расширен и не сужен. + +## Как проверялось + +Дешёвые гейты (прогнаны заново, код после ревизии 2 менялся): + +| Команда | Результат | +|---|---| +| `npx tsc --noEmit` | чисто, без вывода | +| `npm test` | `1338 tests, 1337 pass, 1 skip, 0 fail` | +| `npm run build` | сборка ок; `cp dist/... custom_components/.../frontend/` + `npm run bundle:sync` — три копии бандла побайтово совпадают (`md5sum` сверен), `git status` после пересборки чист | +| `node scripts/check-docs.mjs` | `Documentation checks passed (7 files, 10 external links)` — обязателен, diff трогает `src/**` | + +Смок-выборка. `node scripts/smoke-select.mjs --base $(git merge-base origin/dev HEAD) --head HEAD` +дал 6 прямых совпадений + `smoke_optimize_geometry_preflight.mjs` (сам изменён +в диффе, назван в AC5). Прогнаны все 7: + +- `demo/smoke_preflight_diagnostics.mjs` (новый, AC1–AC6) — **OK** +- `demo/smoke_optimize_geometry_preflight.mjs` (изменён в диффе, эскейпинг/AC5) — **OK** +- `demo/smoke_optimize_coordinate_canonicalization.mjs` — **OK** +- `demo/smoke_general_settings.mjs` — **OK** +- `demo/smoke_help_affordance.mjs` — **OK** +- `demo/smoke_partition_openings.mjs` — **OK** +- `demo/smoke_room_resize.mjs` — **OK** + +«Слабую связь» (8 файлов, одно распространённое имя `_alignDialog`) не +прогонял — конкретно `smoke_optimize_geometry_preflight.mjs` из этой группы +уже покрыт выше как прямо изменённый; остальные не трогают отказную ветку +диалога Optimize. + +Мутанты. Три новых мутанта AC7 — точечно, `node scripts/mutation-gate.mjs --id=`: +`preflight-reason-lost-in-dialog`, `preflight-diagnostics-without-reason`, +`preflight-dev-log-disabled` — все три: чистый прогон зелёный, мутант красный +(«покраснел, как обязан»), поймано 1/1 каждый. Полный набор мутантов не +гонял — не относится к диапазону задачи, дорог, дал сбой на несвязанном +backend-мутанте из-за отсутствия `pytest` в этом окружении (см. «чего не +проверял»). + +`node --test test/single-source-numbers.test.mjs` — 3/3 OK. Диагностический +блок не дублирует ни одно измеренное число: `spaces`/`more` (топ-строка, +первые 3) и новый список причин (первые 10) — производные одного и того же +`d.preflight.failures`, тот же паттерн, что уже используют `liveNames`/ +`visibleLiveNames` и `referenceDetails`/`visibleDetails` в этом же методе; +не новая находка. + +`npm run golden:verify` — **129/129 passed**, включая обе изменённые сцены +(`optimize-preflight-dialog-dark-en`, `optimize-preflight-dialog-light-ru`). +Обязателен: diff меняет видимый рендер диалога. Коммит `822e6223` несёт оба +обязательных трейлера (`Release:`, `Baseline-Reviewed:` → CI run 32940625718), +происхождение эталонов подтверждено двумя независимыми зелёными прогонами CI +(байт-идентичные кадры). + +Инварианты модели (`npm run invariants`) — **не гонял, обоснованно**: diff +трогает `plan-geometry-preflight.ts` только для добавления поля `detail` +(класс перехваченного исключения) в уже существующий результат проверки; +ни рёбра комнат, ни записи толщины, ни `layout`/`marker.space`/`open_spans` +не меняются, вызовы `wallBodiesGeometry`/`floorFootprintGeometry` и их +аргументы — те же. Подтверждено чтением всего файла. + +Backend. Локально `.venv-backend` нет (не облачный агент), `python -c +"import homeassistant"` — `ModuleNotFoundError`, локальный pytest без HA +молча пропускает `test_ha_*.py` — не запускал, результат был бы +недоказательным. Вместо этого: CI run 32942935178 (ревьюируемый HEAD) — +`backend` помечен `skipped` через механизм переиспользования (`#208`, +джоб `reuse`, лог «backend не прогоняется: входы побайтово те же, что в +предыдущем успешном прогоне»), содержимое `.reuse-marker` — cache-ключ на +хэш backend-relevant путей. Прогон, где backend реально исполнился и +включал именно это изменение (новый ассерт `integration_version == +VERSION` в `tests_backend/test_ha_websocket.py`, импорт `VERSION` в +`websocket_api.py`) — run [32939996348](https://github.com/Matysh/houseplan-card/actions/runs/32939996348), +джоб `backend`: **success**. Байт-идентичность входов между этим прогоном и +ревьюируемым HEAD — гарантия самого механизма `#208`, не моя реконструкция +по SHA (промежуточные коммиты переписаны force-push’ем и недоступны локально +для прямой сверки). + +## Находки + +### M1 (в скоупе, чинится в этой же задаче) — диагностика меряет не тот отпечаток + +`_preflightDiagnostics` (`src/houseplan-card.ts:15840-15862`) строит +`spacesById` из **`this._serverCfg`** (уже сохранённая, текущая +конфигурация) и считает `spaceGeometryFingerprint = +spacePhysicalGeometryFingerprint(spacesById.get(failure.spaceId))` от неё: + +```ts +private _preflightDiagnostics(preflight: OptimizeGeometryPreflightResult): object { + const spacesById = new Map((this._serverCfg?.spaces || []) + .map((space: any) => [String(space?.id || ''), space])); + ... + spaceGeometryFingerprint: spacesById.has(failure.spaceId) + ? spacePhysicalGeometryFingerprint(spacesById.get(failure.spaceId)) + : null, +``` + +Но preflight, который эта функция описывает, всегда посчитан от +**кандидата** — `r.config`/`d.config`, результата `optimizePlans`, а не от +`_serverCfg`: + +- `_previewAlignDialog` (`:15977-16007`): `const preflight = r.changed ? + this._checkOptimizeGeometry(r.config) : null;` — preflight существует + **только когда `r.changed === true`**, то есть кандидат по определению + отличается от сохранённой конфигурации; +- `_serverCfg` присваивается из `d.config` только на **успешном apply** + (`:16044`, внутри ветки, которая требует зелёного preflight и не + выполняется для отказа) — то есть в момент, когда пользователь видит + отказ и жмёт «Скопировать диагностику», `_serverCfg` гарантированно несёт + **старую**, дооптимизационную геометрию пространства, а не ту, что + реально проверялась и провалилась. + +Смок `smoke_preflight_diagnostics.mjs` этого не ловит: он подставляет +`_alignDialog = { ..., config: card._serverCfg, ... }` (`:48` — `config: +card._serverCfg`), то есть искусственно делает кандидат равным сохранённой +конфигурации — расхождение, которое существует в реальном потоке, тестовым +фикстурой стёрто. + +**Воспроизведение (по коду, не мутантом):** пространство «F1» имеет комнату +с сохранённым `poly`. Optimize предлагает изменить эту геометрию (сдвиг +узла на сетку) и получает `wall-degraded-extra` на пересчитанной версии. +`checkOptimizeGeometry` вызывается с `r.config` (новый `poly`), возвращает +`fingerprint` от кандидата — верно. Но при клике «Скопировать диагностику» +`_preflightDiagnostics` смотрит в `this._serverCfg.spaces` — **старый** +`poly` — и кладёт в буфер `spaceGeometryFingerprint`, посчитанный от +геометрии, которая preflight вообще не проверял. + +**Почему это не мелочь.** Это прямое нарушение заявленного в ТЗ §1.4/AC4 +смысла поля: «`origin: runtime` — явное признание AC4: отказ зависит от +состояния карточки, экспорт его может не воспроизводить; блок несёт то, +чего в экспорте нет (отпечатки...)». Экспорт пространства сериализует ровно +`_serverCfg`-геометрию. Если `spaceGeometryFingerprint` в диагностике тоже +посчитан от `_serverCfg`, он **совпадёт** с тем, что дал бы хэш экспорта — +то есть не несёт ничего, чего в экспорте нет, ровно для той величины, +которая должна была это доказывать. Это тот же класс дефекта, из-за +которого исходный баг-репорт владельца был недиагностируем (экспорт молчит +о причине) — только теперь неверна не причина, а единственное поле, +которое должно было отличать «кандидат» от «сохранено». + +**Исправление в скоупе задачи:** источник для `spacesById` — конфигурация, +которую реально проверял preflight (`this._alignDialog?.config` в момент +рендера/копирования, либо явно прокинуть `config` параметром в +`_preflightDiagnostics`/`_reportPreflightFailure` из обоих вызывающих мест, +где `r.config`/`d.config` уже есть в области видимости). Изменение +локальное, не требует новой архитектуры. + +### M2 (в скоупе, чинится в этой же задаче) — фолбэк-блок диагностики не сбрасывается и может показать чужой отказ + +`_preflightClipboardFallback` устанавливается в `_copyPreflightDiagnostics` +(`:15889-15899`) и сбрасывается в `null` **только** при успешном +`navigator.clipboard.writeText`. Он не сбрасывается: + +- при открытии диалога Optimize заново (`_previewAlignDialog`, `:15977`, + вызывается и из `_openAlignDialog`, и из `_toggleOptimizeLivePositions`) — + свежий `preflight` кладётся в `_alignDialog`, но старое значение + `_preflightClipboardFallback` остаётся; +- при закрытии диалога (все точки `this._alignDialog = null`: `:2552`, + `:6483`, `:16059`, `:17164`, `:17327`). + +Рендер условия (`:17184`) не привязан к текущему `_alignDialog`/ +`preflight.fingerprint` — `${this._preflightClipboardFallback ? html\`
...\` : nothing}` показывает **любое** непустое значение поля. + +**Сценарий.** Карточка открыта в контексте, где `navigator.clipboard` +недоступен или бросает исключение — ровно тот случай, для которого фолбэк +и написан (ТЗ §1.3/Риск 1: «Clipboard в HA-вебвью/insecure context — +фолбэк обязателен»; это не редкий крайний случай, а ожидаемая среда +эксплуатации). Пользователь получает отказ preflight на пространстве A, +жмёт «Скопировать диагностику», clipboard бросает — виден инлайн-блок с +JSON пространства A. Пользователь закрывает диалог (или диалог закрывается +сам после починки одной проблемы), правит план, снова жмёт «Оптимизировать» +— на этот раз отказ на пространстве B (другая причина). Диалог корректно +показывает новый список причин (это управляется `d.preflight.failures`, +свежим), но блок `
` под кнопкой **всё ещё содержит JSON
+пространства A** — до тех пор, пока пользователь не нажмёт «Скопировать»
+ещё раз. Если он не заметит и скопирует/сфотографирует видимый блок в
+отчёт об ошибке — в отчёт попадёт диагностика **не того** отказа.
+
+Это прямо противоречит цели задачи: диагностируемость деградирует до
+состояния, близкого к исходному дефекту («отчёт непригоден для
+диагностики»), только теперь ошибка не в отсутствии данных, а в неверных
+данных, что хуже — неверный диагноз выглядит как диагноз.
+
+**Исправление в скоупе задачи:** сбрасывать `_preflightClipboardFallback =
+null` в начале `_previewAlignDialog` (или в каждой точке закрытия
+`_alignDialog`), либо привязать видимость блока к совпадению
+`preflight.fingerprint` с тем, для которого блок был построен.
+
+Оба M1/M2 — Medium, в скоупе задачи (часть контракта §1.3/§1.4, тот же
+файл, тот же диалог), правятся без изменения архитектуры; High нет →
+вердикт жёлтый, отдельный issue не заводится (#202).
+
+## Что проверено и корректно
+
+- **AC1** (причина + пространство в диалоге, все 7 reason) — i18n-ключи
+  `gs.preflight_reason_*` присутствуют в RU и EN парно, маппинг
+  `reason → ключ` полный (все 7 значений `OptimizeGeometryFailureReason`
+  использованы), подтверждено юнитом (полный набор) и смоком (два reason
+  из семи видимы в тексте диалога, включая экранирование инъекции в title —
+  сохранено из прежнего теста и расширено).
+- **AC2** (dev-лог несёт spaceId/reason/detail/отпечатки) — единая точка
+  `_reportPreflightFailure`, вызывается из обоих мест вычисления preflight
+  (`:15994`, `:16026` — второй вызов при расхождении fingerprint), дедуп по
+  `preflight.fingerprint` подтверждён смоком (`devLogOnce`) и мутантом
+  (`preflight-dev-log-disabled`). Форма записи проверена смоком
+  (`devLogShape`). *Само содержимое `spaceGeometryFingerprint` внутри этой
+  записи несёт тот же дефект M1* — dev-лог тоже читает `_serverCfg`, не
+  кандидат.
+- **AC3** (кнопка копирует блок, фолбэк при отказе clipboard) — оба пути
+  смоком: успешный `writeText` даёт валидный JSON в буфер, брошенное
+  исключение — инлайн `
` с тем же JSON. Сам факт наличия
+  фолбэка и его контент (кроме M2-дефекта устаревания) корректны.
+- **AC4** (блок называет рантайм-природу: `origin: runtime`, `checkedAt`,
+  отпечатки присутствуют) — поля на месте, типы верны (юнит/смок). Как
+  показывает M1, **значение** одного из отпечатков не достигает
+  заявленного смысла поля — AC в узком, «поле присутствует» прочтении
+  выполнен; в прочтении, для которого поле придумано, — нет.
+- **AC5** (сцена с ломаной геометрией даёт `failed` с причиной до текста
+  диалога, тест падает на dev) — подтверждено: ни один из новых символов
+  (`_reportPreflightFailure`, `_copyPreflightDiagnostics`,
+  `gs.preflight_reason_wall-exception` и т.д.) не существует в
+  `origin/dev` (проверено `git show origin/dev:... | grep -c` → 0 по всем);
+  вызов `card._reportPreflightFailure(...)` в новом смоке гарантированно
+  бросит на dev-бандле. Смок исполнен на HEAD — зелёный.
+- **AC6** (условный «обновите») — `_preflightVersionsDiffer()` сравнивает
+  `_haIntegrationVersion` (из `houseplan/config/get`, три места
+  присвоения — начальная загрузка, warm remount, layout-reload — все три
+  проверены чтением) с `CARD_VERSION`; backend отдаёт тот же `VERSION`, что
+  `import_export.py:526` (общий источник, не дублирующее вычисление).
+  Смок проверяет обе ветки (`noUpdateHintSameVersion`,
+  `updateHintOnDiff`). `_haIntegrationVersion`/`_canOptimizeUndo`/
+  `_undoKind` не входят в реактивный `properties`-список Lit — это
+  существующий в проекте паттерн (не новый риск), значение читается уже
+  после того, как окружающий код-путь вызывает `requestUpdate()` или меняет
+  настоящее reactive-поле.
+- **AC7** (три мутанта) — все три красные при мутации, зелёные без неё,
+  проверено исполнением `--id=`, не только по описанию гварда.
+- **Граница приватности #199** — `detail` = `error.name`/`typeof`, никогда
+  `error.message`; три существующих `assert.doesNotMatch` не менялись и
+  остались зелёными, добавлена позитивная проверка `detail === 'Error'`.
+  Прочитан весь `plan-geometry-preflight.ts` — других мест, где текст
+  исключения мог бы просочиться, нет.
+- **Трейлеры** — `Issue: #295` на всех нетривиальных коммитах;
+  `User-Visible: yes` на `1cdd4ed6` — оба `docs/CHANGELOG.md` и
+  `docs/CHANGELOG.ru.md` правлены **в том же коммите**; `Release:` +
+  `Baseline-Reviewed:` на `822e6223` (эталоны) — оба присутствуют и ссылка
+  на реальный CI-прогон, не выдумана.
+- **USER-GUIDE.ru.md** — новый абзац использует ту же формулировку кнопки
+  («Скопировать диагностику»), что и i18n-ключ `gs.preflight_copy`;
+  терминология не изобретена.
+- **Golden** — 129/129, включая обе изменённые сцены; эталоны приняты
+  штатно (`npm run golden:accept -- --reviewed` со ссылкой на полный
+  Linux CI-артефакт), не «лишь бы зелёный».
+- Скоуп не расширен относительно ревизии 2 спецификации; не-скоуп (сам
+  рантайм-отказ владельца, механика барьера #199, внерешёточная запись,
+  экспортный формат) не тронут — подтверждено диффом.
+
+## Чего не проверял
+
+- Полный набор `demo/smoke_*.mjs` (193 файла) — прогнал 7 (прямые
+  совпадения + изменённый файл), «слабую связь» (8, все — общее имя
+  `_alignDialog`) не гонял: ни один из них не рендерит отказную ветку
+  диалога Optimize и не тестирует preflight-специфичный путь. Полный набор
+  — обязанность пре-релизного гейта, не этого ревью.
+- `python -m pytest tests_backend -q` — не гонял локально (нет
+  `.venv-backend`, нет `homeassistant`, результат был бы недоказательным
+  по молчаливому skip `test_ha_*.py`). Опирался на CI: ревьюируемый HEAD
+  переиспользует (`#208`) результат прогона 32939996348, где `backend`
+  реально исполнился и был зелёным с тем же тестовым файлом.
+- Полный `npm run mutation-gate` (без `--id`) — не гонял: дорого, вне
+  диапазона задачи (диф не трогает другие мутанты), и локальное окружение
+  не имеет `pytest` для backend-мутантов этого гейта (не связано с #295).
+  Три релевантных мутанта прогнаны точечно и красны, как обязаны.
+- `npm run invariants` — не гонял, diff не меняет геометрическую модель
+  (обоснование в разделе «Как проверялось»).
+- Performance-профили — не названы в AC, не затронуты (diff добавляет код
+  только в отказную ветку уже вычисленного preflight; ok-путь не получил
+  ни одной новой операции — подтверждено чтением, не измерением).
+- Ручное тестирование в браузере HA — вне цикла ревью по процессу; замену
+  ему составили смоки и golden.
+
+---
+
+Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 2 → в задаче