From 3cb9e2ba6237f9de848cd44eb09982f7b230f1de Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Fri, 28 Aug 2026 04:58:13 +0000 Subject: [PATCH] docs: review document for #337 Issue: #337 User-Visible: no --- docs/reviews/CODE-REVIEW-337-r2.md | 248 +++++++++++++++++++++++++++++ 1 file changed, 248 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-337-r2.md diff --git a/docs/reviews/CODE-REVIEW-337-r2.md b/docs/reviews/CODE-REVIEW-337-r2.md new file mode 100644 index 00000000..af84c6be --- /dev/null +++ b/docs/reviews/CODE-REVIEW-337-r2.md @@ -0,0 +1,248 @@ +# CODE-REVIEW-337-r2 + +- Issue: https://github.com/Matysh/houseplan-card/issues/337 +- Материал: `git diff 6d338b78..HEAD` (r1 SHA `6d338b78a0794491639edee9171662cdd260c42b` → + HEAD `2beafc06398c16d1cad80fddbe77e6a40f111b9a`), ветка `issue/337-lazy-editor-chunk`. + 4 коммита, 32 файла (без учёта сгенерированных `dist/*`/`custom_components/houseplan/frontend/*`, + которые зеркалят исходники): `src/houseplan-card.ts` (+79/-13), `src/houseplan-editor-runtime.ts` + (+55/-55, преимущественно механическая правка), `scripts/mutation-gate.mjs`, два unit-теста, + 11 browser-smoke фикстур, документ CODE-REVIEW-337-r1, обновлённый `docs/images/screenshots.json`. +- Этап: код-ревью, заход r2, блокирующих циклов израсходовано 1/4 до этого разбора. +- **Вердикт: красный.** + +## Скоуп разбора + +Предыдущий вердикт (CODE-REVIEW-337-r1, SHA `6d338b78`) — красный, 5 High + 3 Medium. +Дельта этого раунда — точечные правки по каждой находке, а не переработка задачи: +два изменённых src-файла, обновлённые smoke-фикстуры под новый async-контракт, +одна правка mutation-gate (стал ссылаться на актуальный файл стилей после переноса +CSS), плюс сам документ r1 и пересборка bandle-артефактов. Это не ребейз на ушедший +`dev` (branch и так уже стоит на `508945c0` = `origin/dev`), не смена контракта +поведения и не новая подсистема — разбор веду по дельте, а не с нуля, но +дополнительно перепрогоняю **весь** browser-smoke census и `golden:verify`, а не +только смоки, названные автором: H4-фикс (`_deviceInboxTabKey`/`@click=${this._x}` +→ `@click=${() => this._x()}`) механически переписал практически КАЖДЫЙ +обработчик события в `houseplan-editor-runtime.ts` (не только вкладки инвентаря +устройств) — это затрагивает общий рендер-путь всех редакторских диалогов, а не +только те четыре сценария, что назвал автор. + +## Как проверялось + +Дешёвые гейты (перепрогнаны самостоятельно, а не приняты со слов автора): + +| Гейт | Команда | Результат | +|---|---|---| +| typecheck | `npx tsc --noEmit` | pass (5.6s) | +| unit | `npm test` | 1439 total, 1438 pass, 1 skip, 0 fail — совпадает с заявлением автора | +| build+sync+budget | `npm run build && npm run bundle:sync && npm run bundle:budget` | initial View 255 778 B ≤ 256 000 B (запас 222 B — **уже, чем 615 B на r1**); lazy editor 131 785 B; `bundle:sync` не оставил расхождений в дереве (только биты прав доступа 755→644 на двух артефактах из-за локального `tsc`, отменено `git checkout --`, содержимого не касалось) | +| docs fingerprint | `node scripts/check-docs.mjs` (diff трогает `src/**`) | pass (7 файлов, 10 внешних ссылок) | +| mutation, changed-range | `node scripts/mutation-gate.mjs --check --changed=6d338b78..HEAD` | 58/58 применимых guard'ов поймали подмену | + +Тяжёлые гейты — по дельте и по решению ревьюера, не выборочно на веру: + +- **Полный browser-smoke census** (195 файлов, `for f in demo/smoke_*.mjs; do + node "$f"; done` после `bundle:sync`) — **195/195 pass**, включая + `smoke_warm_dialogs` (единственный прогон в составе census). Это ожидаемо и + НЕ противоречит находке H6 ниже: точечный прогон того же файла 7 раз подряд + дал 5 отказов из 7 (≈71% красных) — при однократном прогоне в общем census + благополучный исход (≈29% по моей выборке) не редкость. Единичный зелёный + census — статистический шум для гонки такой частоты, а не доказательство + исправности; полагаться на однократный прогон здесь означало бы повторить + саму ошибку хендоффа автора. +- **`npm run golden:verify`** — 127 passed / **4 different** — те же четыре + кадра, что в r1: `geometry-devices-editor-dark`, `device-dialog-mobile-ru`, + `toggle-entity-dialog-mobile-ru`, `device-ripple-color-popover-mobile-ru`. +- **Независимая проверка авторского заявления по H5** (не принято на веру: автор + утверждает «pre-existing stale baseline, не регресс #337» — это фактическое + утверждение о состоянии `dev`, которое легко проверить и легко сфальсифицировать). + Поднял `git worktree add /tmp/dev-check origin/dev --detach` (чистый `508945c0`, + ancestor этой ветки), `npm ci && npm run build && npm run bundle:sync`, затем + `node demo/golden/run.mjs --mode=capture --scenario=` (verify требует полную + матрицу, capture позволяет точечный diff) для всех четырёх кадров: + **все четыре «different» воспроизводятся на чистом `dev` без единого изменения + из #337.** Открыл сами diff-PNG: `geometry-devices-editor-dark` — актуальный + рендер показывает два раздельных, не наложенных друг на друга элемента + тулбара («Devices», «Icon rules»); диффовое наложение возникает потому что + **эталон** старше и содержит три кнопки («Add», «Hidden and disabled», + «Icon rules») — устаревший baseline `dev`, а не порча layout веткой. Три + mobile-ru кадра показывают одинаковую узкую полосу по правому/нижнему краю + диалога и на чистом `dev` — тоже baseline drift, не регресс. **H5 закрыт**: + находка воспроизводится на `dev` и не относится к #337; это отдельный, + предсуществующий дефект золотых эталонов — завёл отдельный issue + ([#346](https://github.com/Matysh/houseplan-card/issues/346)). +- **Целевые smoke, привязанные к находкам r1**, каждый прогнан минимум по + разу, некоторые — многократно при подозрении на нестабильность (ниже): + `smoke_preloader_lifecycle`, `smoke_warm_owners`, `smoke_kiosk`, + `smoke_room_resize`, `smoke_optimize_geometry_preflight`, + `smoke_device_inbox`, `smoke_fixed_floor`, `smoke_orphan_space_references`, + `smoke_general_settings`, `smoke_ha_controls`, `smoke_nav_persist`, + `smoke_partition_openings`, `smoke_grid_scale_invariance`, + `smoke_edit_walk`, `smoke_help_affordance`, `smoke_opening_entity_search`. +- **`smoke_warm_dialogs` — прогнан 7 раз подряд после `bundle:sync`, а не один + раз.** Причина повторов: первый запуск дал `bViewBitExact` FAIL на кадре, + близком, но не идентичном сохранённой камере — при плавающей точке одного + разового отказа недостаточно, чтобы отличить гонку от единичной аномалии + окружения. Результат: **5 из 7 запусков красные**, см. находку H6 ниже. Это + прямо противоречит заявлению автора «warm_dialogs green» в хендоффе. +- **model-invariants** — не запускал: дельта не меняет ни один формат хранения + геометрии (`layout`, `marker.space`, `open_spans`, ключи толщины); H3-фикс + меняет только то, ЧЕРЕЗ КАКОЙ объект (`this` vs `this.host`) вызывается уже + существующая preflight-проверка, не сами данные. Не тот инструмент для этой + дельты — то же обоснование, что в r1. +- **Backend/pytest** — не тронуты этой дельтой (`custom_components/**/*.py` не + входит в diff `6d338b78..HEAD`), не перепрогонял. + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где видно | +|---|---|---| +| **H1** (warm-remount коммитит mode в обход `_ensureEditorRuntime()`) | `_warmAdoptViewport` больше не коммитит редакторский `mode` напрямую: адаптирует `'view'` синхронно, а восстановление `mode` уходит через `_requestMode(restoredMode, false, true)`, который проходит `_ensureEditorRuntime()`; новый параметр `adopt` в `_requestMode` атомарно вызывает `_adoptMode` + возобновляет отложенный `_warmReviveDialog` | `src/houseplan-card.ts:890-901` (сигнатура/adopt-ветка `_requestMode`), `:3190-3222` (`_warmAdoptViewport`), `:3358-3369` (`_warmReviveDialog` откладывает потребление диалога, если ждёт runtime) | **Частично** — крах устранён (`smoke_preloader_lifecycle`, `smoke_warm_owners` зелёные детерминированно), но см. новую находку **H6**: та же зона правки сломала бит-точность камеры | +| **H2** (kiosk scale зависит от lazy runtime) | `_saveKioskScale`/`_renderKioskDialog` возвращены в eager-код хоста, больше не форвардятся в `_editorRuntimeOrThrow()`; вызов диалога больше не защищён условием `this._editorRuntime ?` | `src/houseplan-card.ts:10629-10662`, `:11350` (`${this._kioskDialog ? this._renderKioskDialog() : nothing}`) | `smoke_kiosk` зелёный (сам прогнал) | +| **H3** (fail-closed resize/optimize preflight не перехватывается подменой) | Внутренние вызовы `_checkSpacePhysicalGeometry`/`_checkOptimizeGeometry` внутри runtime переведены на `this.host._checkSpacePhysicalGeometry(...)`/`this.host._checkOptimizeGeometry(...)` — то есть идут через тот же публичный метод хоста, который переопределяет смок; сами реализации переименованы в `..._Impl` и не вызываются напрямую нигде, кроме хоста | `src/houseplan-editor-runtime.ts:1117-1120` (интерфейс `HouseplanEditorHostPort` пополнен), `:1761, 2085, 2156, 2265, 2288, 3609, 3626, 8591, 8599, 8688, 8721` (все внутренние вызовы через `this.host.`), `src/houseplan-card.ts:9973/9984` (`_checkOptimizeGeometry`/`_checkSpacePhysicalGeometry` хоста форвардят в `..._Impl`) | `smoke_room_resize` и `smoke_optimize_geometry_preflight` зелёные (сам прогнал) | +| **H4** (`_deviceInboxTabKey` падает — `this` не тот) | Метод стал полем-стрелкой (`public _deviceInboxTabKey = (event) => {...}`), а КАЖДЫЙ голый `@event=${this._method}` в файле переписан в `@event=${() => this._method(...)}` — то есть устранён весь класс бага, а не только один обработчик; закреплено регрессионным unit-тестом, который grep'ает файл на голые `@x=${this._y}` и требует пустой список | `src/houseplan-editor-runtime.ts:7401-7410` (метод), десятки мест по файлу (см. `git diff 6d338b78..HEAD -- src/houseplan-editor-runtime.ts`), `test/editor-runtime-loader.test.mjs:124-133` («lazy runtime event handlers keep the runtime receiver») | `smoke_device_inbox` зелёный (сам прогнал); тест зелёный в `npm test` | +| **H5** (4 golden-дельты, включая порчу тулбара Device editor) | Не изменено ни строки, относящейся к golden — **не регресс #337**: independently подтверждено, что все 4 кадра дают «different» и на чистом `origin/dev` (`508945c0`) без единого коммита этой ветки, см. раздел «Как проверялось» | `git diff origin/dev...HEAD -- demo/golden/baselines/` пуст (нулевой diff эталонов); скриншот-diff `geometry-devices-editor-dark` на чистом `dev` показывает то же наложение старого 3-кнопочного baseline поверх нового 2-кнопочного актуального рендера | **Закрыт как не относящийся к #337**; отдельный issue [#346](https://github.com/Matysh/houseplan-card/issues/346), не блокирует эту задачу | +| **M1** (`fixed_floor`/`orphan_space_references` создают `houseplan-card-editor` напрямую) | Обе фикстуры переведены на `await customElements.get('houseplan-card').getConfigElement()` | `demo/smoke_fixed_floor.mjs:75`, `demo/smoke_orphan_space_references.mjs:191` | Оба смока зелёные (сам прогнал) | +| **M2** (`general_settings` ищет версию только в entry-файле) | Смок теперь читает `houseplan-assets.json`, конкатенирует все `.js`-файлы манифеста и ищет баннер в объединённой строке | `demo/smoke_general_settings.mjs:61-67` | Зелёный (сам прогнал) | +| **M3** (9 нерасследованных красных смоков) | `ha_controls`, `nav_persist`, `partition_openings`, `grid_scale_invariance` явно адаптированы под async editor runtime (`_editorRuntime`, `await c._requestMode(...)`, `await c._ensureEditorRuntime()`) | `demo/smoke_ha_controls.mjs`, `demo/smoke_nav_persist.mjs`, `demo/smoke_partition_openings.mjs`, `demo/smoke_grid_scale_invariance.mjs` (везде в диффе `6d338b78..HEAD`) | Все четыре зелёные при точечном прогоне; `edit_walk`, `help_affordance`, `opening_entity_search` не изменялись в дельте — перепрогнаны отдельно, зелёные, входят в «унаследовано» ниже | + +## Новые находки + +### High (блокирует) + +**H6. Warm-remount editor-камера теряет бит-точность — визуальный «прыжок» +кадра, воспроизводится в большинстве прогонов.** + +Контракт (комментарий самого смока, `demo/smoke_warm_dialogs.mjs:1-9`, +`docs/WARM-REMOUNT.md`): при пересоздании элемента картой (Lovelace warm +remount) камера редактора должна восстановиться **бит-в-бит — ни один кадр не +отличается от прежнего**. Прогнал `node demo/smoke_warm_dialogs.mjs` **7 раз +подряд** после `npm run bundle:sync`: **5 из 7 — FAIL** на `bViewBitExact`: + +``` +FAILED (1): + - bViewBitExact: expected true, got "кадр 20мс: zoom=3.4 + view=[256.7064599980745,247.64705882352942,346.587080003851,264.70588235294116] + (ждали 3.4 / [254.1156927887434,247.64705882352942,351.7686144225132,264.70588235294116])" +``` + +Во всех пяти отказов кадр приходит на 9–20мс после пересоздания, `zoom` и `y/h` +точно совпадают с ожиданием, а `x`/`w` — нет (расхождение 2–6 логических +единиц) — то есть не полный сброс камеры, а короткий одиночный кадр с +пересчитанной шириной вида, прежде чем всё встаёт на место. Это ровно та зона, +что переписал H1-фикс: `_warmAdoptViewport` (`src/houseplan-card.ts:3196-3222`) +синхронно восстанавливает `_zoom`/`_view` в точные сохранённые числа ДО того, +как `_mode` меняется на `'devices'`, но сам `_mode` первый кадр или два остаётся +`'view'` (полноширинная сцена без редакторского тулбара), а асинхронный +`_requestMode(restoredMode, false, true)` переключает `_mode` только после +`_ensureEditorRuntime()`. Между этими двумя моментами существует окно, где +камера уже стоит в «devices»-числах, но сцена ещё рендерится в View-раскладке +(другая доступная ширина stage) либо наоборот — и какой-то путь размера/layout +(вероятно, привязанный к `ResizeObserver`/первому измерению `_stageEl` для +нового инстанса, `_lastValidStageSize` пуст на свежесозданном элементе) +пересчитывает `x`/`w` под несовпадающую раскладку на один кадр. + +**Смок умеет падать** — это не переписанный этой задачей файл, докстрока прямо +говорит «ПАДАЕТ на сборке до DEV-B703-03» (задача, которая изначально ввела +это требование), и я лично наблюдал детерминированный, воспроизводимый по +большинству прогонов красный результат, а не разовую аномалию: 5/7. Полный +census (195/195, единичный прогон каждого файла) этот файл не поймал — +статистически ожидаемо при частоте отказа 71% и единственной попытке (см. раздел +«Как проверялось»), а не признак исправности. + +Это отдельная находка от H1 (не то же самое, что было в r1): в r1 весь смок падал +с необработанным исключением ДО того, как код доходил до этой проверки — +H1-фикс устранил исключение, но обнажил под ним ещё один, ранее не наблюдаемый +дефект в том же коде. Прямо противоречит хендоффу автора («warm_dialogs... +green», перечислено как pass в списке «targeted review smokes»). Учитывая +majority-red частоту (71%) и то, что визуальный прыжок камеры при +warm-remount — это ровно тот UX-дефект, ради предотвращения которого +существует весь контракт DEV-B703-03/WARM-REMOUNT.md, серьёзность максимальная. + +### Medium (вне скоупа #337 — отдельный issue) + +**M4 (→ [#346](https://github.com/Matysh/houseplan-card/issues/346)). Golden-эталоны +`geometry-devices-editor-dark`, `device-dialog-mobile-ru`, +`toggle-entity-dialog-mobile-ru`, `device-ripple-color-popover-mobile-ru` +устарели на `origin/dev` независимо от #337.** Воспроизведено на чистом +`508945c0`: `node demo/golden/run.mjs --mode=capture --scenario=` для +каждого из четырёх даёт `different`; diff-PNG `geometry-devices-editor-dark` +показывает наложение устаревшего 3-кнопочного тулбара эталона на актуальный +2-кнопочный рендер (тулбар Device editor был упрощён каким-то более ранним, +уже смёрженным в `dev` изменением, после которого эталон не переснимали); три +mobile-ru кадра — устаревший размер диалога по правому/нижнему краю. Нужно +принять новые эталоны через `npm run golden:accept -- --reviewed` по чистому +`dev`, не через эту ветку. Issue заведён. + +## Что проверено и корректно + +- **Бюджет (AC1).** 255 778 B ≤ 256 000 B, посчитано лично. Запас упал с 615 B + (r1) до 222 B — риск, названный ещё в r1, материализуется: правки H1/H4 + добавили код (новый параметр `_requestMode`, обёртки обработчиков), и запас + почти исчерпан. Не блокирую этим, так как AC1 формально выполнен и риск уже + зафиксирован, но следующая правка (в том числе исправление H6) обязана + держать бюджет в уме. +- **H2/H3/H4/M1/M2 фактически исправлены** — не только по диффу, но по + собственному прогону соответствующих smoke, см. таблицу выше. +- **H5 закрыт корректно и добросовестно** — авторское расследование не просто + заявлено, а воспроизведено мной независимо на чистом `dev`. +- **Мутационное покрытие затронутых guard'ов (changed-range)** — 58/58 ловят + подмену; changed-range корректно расширился под новый файл + `src/styles/plan.styles.ts`, где теперь реально лежит проверяемый CSS. +- **Полный browser-smoke census** — 195/195 pass на единичном прогоне (не + противоречит H6, см. выше). +- **Trailers/changelog.** Все 4 коммита несут `Issue: #337`; все — `User-Visible: + no` — корректно, т.к. пользовательское поведение (после исправления + H1–H4) возвращено к состоянию ДО дефектов r1, changelog для #337 уже писался + отдельным коммитом на предыдущем этапе (`c1aaddc7`, зафиксирован в r1). + +## Унаследовано из r1 + +Принято без повторной проверки в этом раунде, дельта их не касается: + +- **AC7 (backend asset route)** — `tests_backend/test_frontend_assets.py`, + не входит в diff `6d338b78..HEAD`; вывод r1 (CODE-REVIEW-337-r1.md, + SHA `6d338b78`, раздел «Что проверено и корректно») остаётся в силе. +- **AC8 (bundle sync/freshness/HACS zip/release scripts)** — не входит в + дельту; вывод r1 наследуется. +- **AC9 (CSS-минификатор, adversarial fixtures)** — не тронут этой дельтой, + кроме одной ссылки в `mutation-gate.mjs` на новый путь файла стилей, что не + меняет сам минификатор; вывод r1 наследуется. +- **AC10 (fingerprint mismatch)**, **AC12 (no model drift)**, **AC13 (докс не + обещает single-file install)** — вне дельты, наследуются из r1 документа. +- **Прочие смоки, не входящие в таблицу закрытия r1**, включая полный набор + 195 файлов — пройдены заново единичным census + точечными повторными + прогонами там, где я подозревал гонку, а не приняты по r1 без проверки: + дельта тронула общий рендер-путь всех редакторских диалогов (H4-правка), + поэтому я не ограничился списком автора. + +## Чего не проверял + +- **Полный HA-harness backend** (`python -m pytest tests_backend -q`) — + недоступен в этой песочнице (нет `homeassistant`), дельта не трогает + `custom_components/**/*.py`, поэтому и не требовался в этом раунде. +- **`node scripts/model-invariants.mjs` / `npm run invariants`** — не + запускал, дельта не меняет формат геометрической записи (обоснование выше). +- **Perf-профили** — не запускал, не названы в AC и не затронуты дельтой. +- **Ручное тестирование в реальном Home Assistant** — не выполнялось. +- **Частоту гонки H6 в CI-окружении** (headless Linux CI vs моя песочница) — + не измерял отдельно; предполагаю сравнимую или более высокую частоту (CI + раннеры обычно медленнее/более вариативны по таймингу кадров, что скорее + усилит гонку, чем скроет её), но это предположение, а не измерение. + +## Итог + +Три из пяти High r1 закрыты полностью (H2, H3, H4), H5 закрыт как посторонний +(и это подтверждено, а не принято на слово), но **H1 закрыт лишь частично**: +устранённый крах открыл дорогу новому, статистически частому (5 из 7 прогонов) +дефекту в той же самой warm-remount/lazy-adopt зоне — H6, видимый прыжок +камеры редактора при пересоздании карточки. Это блокирующая находка, вердикт +остаётся красным. Заявление автора «warm_dialogs... green» не подтвердилось +при независимом многократном прогоне — рекомендую на следующем заходе +прогонять чувствительные к таймингу smoke (`smoke_warm_dialogs`, +`smoke_warm_owners`, `smoke_preloader_lifecycle`) не менее 3–5 раз подряд +перед хендоффом, а не один раз. + +Отдельно: заведён новый issue [#346](https://github.com/Matysh/houseplan-card/issues/346) +на предсуществующий (не из #337) дефект золотых эталонов Device +editor/mobile-ru диалогов (M4) — он не блокирует #337 и не должен чиниться в +этой ветке.