mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
claude[bot]
@@ -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<N-1>» / «Унаследовано» не применимы.
|
||||
|
||||
## Скоуп
|
||||
|
||||
Диалог «Оптимизировать» при отказе 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=<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\`<details
|
||||
open>...\` : nothing}` показывает **любое** непустое значение поля.
|
||||
|
||||
**Сценарий.** Карточка открыта в контексте, где `navigator.clipboard`
|
||||
недоступен или бросает исключение — ровно тот случай, для которого фолбэк
|
||||
и написан (ТЗ §1.3/Риск 1: «Clipboard в HA-вебвью/insecure context —
|
||||
фолбэк обязателен»; это не редкий крайний случай, а ожидаемая среда
|
||||
эксплуатации). Пользователь получает отказ preflight на пространстве A,
|
||||
жмёт «Скопировать диагностику», clipboard бросает — виден инлайн-блок с
|
||||
JSON пространства A. Пользователь закрывает диалог (или диалог закрывается
|
||||
сам после починки одной проблемы), правит план, снова жмёт «Оптимизировать»
|
||||
— на этот раз отказ на пространстве B (другая причина). Диалог корректно
|
||||
показывает новый список причин (это управляется `d.preflight.failures`,
|
||||
свежим), но блок `<details><pre>` под кнопкой **всё ещё содержит 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 в буфер, брошенное
|
||||
исключение — инлайн `<details><pre>` с тем же 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 → в задаче
|
||||
Reference in New Issue
Block a user