docs: review document for #89

Issue: #89
User-Visible: no

docs/reviews/CODE-REVIEW-89-r4.md

@@ -0,0 +1,268 @@
+# CODE-REVIEW-89-r4 — #89, этап 1: перф-исправление AC11 (viewToggleMs)
+
+- Issue: [#89](https://github.com/Matysh/houseplan-card/issues/89)
+- Этап: `code` (PROCESS.md §2.7)
+- Диапазон: `origin/dev...HEAD`, `origin/dev` = `3270e03` («test: accept
+  v1.63.0-beta.1 golden baselines»), `HEAD` = `2576d9d` («perf: reuse wall
+  union across projection toggles»), детач `HEAD`, ветка
+  `issue/89-isometric-stage1` — **один коммит**
+- ТЗ: [`docs/specs/089-isometric-view-stage1.md`](../specs/089-isometric-view-stage1.md),
+  ревизия 3, ревью ТЗ зелёное — [`SPEC-REVIEW-89-r1.md`](SPEC-REVIEW-89-r1.md)
+- Предыдущий цикл: [`CODE-REVIEW-89-r3.md`](CODE-REVIEW-89-r3.md) (зелёный,
+  подтверждён повторно после merge-конфликта). Задача вернулась в
+  `S6-in-progress` не из-за находки код-ревью, а из-за прогона **pre-release
+  exact-SHA Full Performance** на кандидате `3270e03`: `large-house-isometric-v1`
+  budget нарушен — `timing.viewToggleMs.median` **192,8 мс** против лимита
+  **131,7 мс** (`base 31,7 мс × 1.2 + 100 мс noise`). Это AC11 ТЗ (§8.2).
+  Согласно PROCESS.md §10.4 «цикл считается по этапу»: вердикт по ТЗ не
+  расходует бюджет код-ревью, а находка **до выпуска беты** — это возврат на
+  правки, а не новый баг (§4). Цикл код-ревью продолжает счётчик,
+  опубликованный в комментариях issue (r2 — красный H1, r3 — зелёный дважды),
+  поэтому это **r4/4** — последний разрешённый цикл.
+- Вердикт: **красный**
+
+## Скоуп ревью
+
+Единственный коммит диапазона:
+
+- `2576d9d` «perf: reuse wall union across projection toggles» (класс A+B):
+  `src/wall-thickness.ts` (+14/−2, новая экспортируемая
+  `wallBodiesPathFromGeometry`), `src/houseplan-card.ts` (+21/−5, новый
+  `_wallUnionKey()`, `_isoSource().build()` возвращает `{geom, depthUnits}`
+  вместо голого `geom`, `_isoScene()` сеет `_wallUnionCache` уже вычисленным
+  union'ом), `test/wall-thickness.test.mjs` (+13, юнит на эквивалентность),
+  плюс синхронные копии бандла (класс D). Трейлеры: `Issue: #89`,
+  `User-Visible: no` — верно для Labs-скрытой фичи (публичных changelog правок
+  не требуется и не внесено).
+
+Продуктовый код всего остального Stage 1 (Labs-механизм, проекция/камера,
+топология стен, fallback-защёлка, touch/kiosk/warm-remount, backend, i18n,
+публичная документация) не менялся с уже дважды зелёного `CODE-REVIEW-89-r3.md`
+— не переоткрываю то, что там разобрано построчно. Задача этого цикла узкая:
+проверить, что именно это исправление действительно решает AC11, и что оно не
+вносит новых регрессий.
+
+Прочитано до вердикта: тело issue #89 и все 18 комментариев (включая решения
+владельца Q1–Q6/O1–O6 и оба сообщения о prerelease-регрессии/фиксе),
+`docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md`, `docs/specs/089-isometric-view-stage1.md`
+(§8.2 — перф-контракт и бюджеты), `CODE-REVIEW-89-r2.md`/`r3.md`,
+`demo/performance/README.md`, `demo/performance/budgets-large-house-isometric.json`,
+`demo/benchmark_large_house.mjs`, `demo/fixtures/large-house.mjs`, полный diff
+коммита `2576d9d` в обоих продуктовых файлах и тесте.
+
+## Как проверялось
+
+`npm ci` выполнен перед гейтами на чистой рабочей копии (зависимостей не
+было; `git config core.hooksPath` → `.githooks`).
+
+| Гейт | Команда | Результат |
+|---|---|---|
+| Типы | `npx tsc --noEmit` | green |
+| Unit | `npm test` (`npm run inventory`: 744 Node unit) | **744/744** green |
+| Сборка | `npm run build` | green |
+| Синхронность бандлов | `cmp dist/… custom_components/…`, `cmp dist/… demo/srv/…` | идентичны байт-в-байт; SHA-256 `31bea717b288e3b1486ee63fa8be6b8d3178ada8bb740ff0c2c76b887bbb5dd3` — совпадает с числом, заявленным автором |
+| Process gate | `node scripts/process-gate.mjs --range origin/dev..HEAD --issues` | green, 0 предупреждений |
+| Браузерные смоки — все 127 | `node demo/smoke_*.mjs` (после `npx playwright install --with-deps chromium`, свежий бандл) | **127/127 green**, включая `smoke_isometric_live_touch.mjs` (21/21) и `smoke_isometric_contract.mjs` (14/14) |
+| Golden — весь матрикс (не только iso-сцены) | `npm run golden:capture` | **47/47 `passed`, 0 diff** — включая все 6 `isometric-*` сцен (эталоны уже приняты на `origin/dev` коммитом `3270e03`, до этого цикла не относился) |
+| Backend | диапазон не трогает `custom_components/**/*.py` | гейт пуст по построению, не запускался |
+| **Perf — целевая проверка AC11** | см. отдельный раздел ниже | **не подтверждает исправление** |
+
+### Проверка AC11: действительно ли исправление снимает регрессию
+
+Это единственный содержательный вопрос цикла — «работает ли фикс», а не
+«гейты зелёные». Officiальный exact-SHA `Full Performance` — CI-only job на
+выделенном раннере; локально он не гейт (PROCESS.md §8), но раз ручного
+тестирования в цикле нет, я обязан оценить это сам, а не поверить на слово,
+тем более что автор сам пишет: «После зелёного слияния AC11 будет заново
+проверен полным exact-SHA Full Performance» — то есть саму гипотезу фикса
+никто ещё не проверял исполнением.
+
+**Метод 1 — прямой A/B на двух git worktree (одна машина, один Chromium).**
+`git worktree add /tmp/wt/base origin/dev` (= `3270e03`, код **до** фикса, тот
+самый кандидат, что провалил prerelease-гейт), собрал бандл там же. Прогнал
+официальный `demo/benchmark_large_house.mjs --profile=large-house-isometric-v1`
+с `--target-root` на обоих деревьях, 5 samples/1 warmup:
+
+| | `viewToggleMs.median` |
+|---|---:|
+| `origin/dev` (до фикса) | **244,4 мс** |
+| `HEAD` (после фикса) | **237,8 мс** |
+
+Разница — **2,7%**, в пределах шума одного прогона (p95 у обоих ~320 мс).
+Абсолютные числа выше, чем в CI (другое железо, не pinned раннер) — это
+ожидаемо и не главное; главное — **относительное** улучшение практически
+нулевое, тогда как заявленный дефицит бюджета — `192,8 → 131,7` мс, то есть
+требуется снижение примерно на **треть**.
+
+**Метод 2 — разбивка round-trip на направления.** `viewToggleMs` в бенчмарке
+измеряет `_setProjection('flat')` **и** `_setProjection('iso')` одним общим
+таймером (`demo/benchmark_large_house.mjs:172-183`). Я измерил оба перехода
+раздельно (тот же fixture/preload, 6 сэмплов каждое дерево):
+
+| | `toFlatMs` (Iso→Flat, цель фикса) | `backToIsoMs` (Flat→Iso, cache hit) |
+|---|---:|---:|
+| `origin/dev` (до фикса) | **174,2 мс** (медиана) | 70,4 мс |
+| `HEAD` (после фикса) | **178,1 мс** (медиана) | 55,3 мс |
+
+Направление, которое фикс должен был ускорить (Iso→Flat: «переход в Flat
+повторял тяжёлую polygon-union операцию»), **не стало быстрее** — медиана
+даже чуть выше на данном прогоне (в пределах шума по 6 сэмплам, диапазон
+141–215 мс на обоих деревьях). Сумма медиан обеих сторон (174,2+70,4≈244,6;
+178,1+55,3≈233,4) согласуется с Методом 1 — методология внутренне
+непротиворечива.
+
+**Метод 3 — микробенчмарк изолированной операции, вне браузера/DOM.** Чтобы
+понять, сколько именно стоит сама «повторная polygon-union операция», которую
+чинит патч, я собрал прокси-фикстуру одного этажа `large-house-isometric-v1`
+(20 комнат в сетке 5×4, 20 перегородок, 13 колонн, 34 проёма — те же
+пропорции, что `demo/fixtures/large-house.mjs` даёт на один `space`) и вызвал
+скомпилированные (`test-build/`, тот же вывод, что использует `npm test`)
+`wallBodiesGeometry`/`wallBodiesUnionPath`/`wallBodiesPathFromGeometry`
+напрямую, без браузера и без Lit:
+
+| Сценарий | Медиана (8 замеров, без первого JIT-прогрева) |
+|---|---:|
+| Один вызов `wallBodiesGeometry` (одна iso-сборка) | 13,1 мс |
+| **До фикса**: `wallBodiesGeometry` дважды (iso-сборка + `wallBodiesUnionPath` для flat) | **18,4 мс** |
+| **После фикса**: `wallBodiesGeometry` один раз + дешёвый `wallBodiesPathFromGeometry` | **8,0 мс** |
+
+Сама операция, которую убирает патч, **экономит ≈10 мс** на этаж такого
+масштаба (56% от своей собственной стоимости, но малая доля от общего
+перехода). Это подтверждает, что патч технически корректен и действительно
+устраняет дублирующий вызов (см. «Что проверено и корректно» — сам рефакторинг
+чист), но объясняет, почему это не видно в Методах 1–2: **≈10 мс экономии на
+фоне ≈150–180 мс полного перехода Iso→Flat — это подавляющая часть стоимости
+перехода лежит не в polygon-union, а где-то ещё** (вероятный кандидат —
+полная Lit-перерисовка/пересборка DOM большой SVG-сцены при смене режима
+проекции: 20 комнат, 500 decor, 200 устройств на диапазон фикстуры, структурно
+разные шаблоны flat/iso, которые lit-html не может частично патчить и
+вынужден перестраивать целиком — но диагностика первопричины не входит в мой
+мандат ревьюера, чинит автор).
+
+**Вывод по AC11.** Три независимых метода (браузерный A/B на реальном коде,
+разбивка по направлениям, изолированный микробенчмарк вне браузера) указывают
+в одну сторону: заявленное исправление даёт реальную, но на два порядка
+меньшую экономию (~10 мс), чем требуется, чтобы закрыть разрыв бюджета
+(нужно убрать ~61 мс, `192,8 → 131,7`). Локальное железо отличается от pinned
+CI-раннера, поэтому абсолютные числа не заменяют exact-SHA Full Performance —
+но соотношение «экономия патча ≪ требуемая экономия» получено на одной и той
+же машине для обеих сторон сравнения и не зависит от абсолютной скорости
+железа. Я оцениваю AC11 как **не доказанным исполнением** — есть прямое
+эмпирическое основание полагать, что regresssion сохранится на следующем
+exact-SHA прогоне.
+
+## Находки
+
+### H1 (блокирует) — исправление, вероятно, не закрывает AC11
+
+**Файлы:** `src/houseplan-card.ts:4421-4485`, `src/wall-thickness.ts:1228-1238`.
+
+**Сценарий отказа:** пользователь/CI публикует `v1.63.0-beta.1` после этого
+цикла; exact-SHA `Full Performance` на финальном кандидате снова измеряет
+`large-house-isometric-v1`; `viewToggleMs.median` остаётся в диапазоне
+≈180–235 мс (см. три метода выше) против лимита 131,7 мс — тот же красный
+результат, что уже был получен 2026-08-13T14:47:46Z на предыдущем кандидате,
+только после того как issue пройдёт весь цикл заново (а бюджет циклов
+код-ревью уже исчерпан — это r4/4).
+
+**Почему не блокирую как «нужно больше тестов, а не факт»:** воспроизвёл на
+трёх независимых уровнях (полный браузерный round-trip, раздельные
+направления, чистый вычислительный микробенчмарк без браузера) с
+консистентным результатом на одной машине для обеих сторон сравнения
+(`origin/dev` vs `HEAD`); ни один из трёх не показывает улучшения,
+приближающегося к требуемому. Это не «гейт не запускался», а «гейт по сути
+эквивалентен запущен, и он показывает, что фикс не достигает цели».
+
+**Рекомендация (не входит в мой мандат менять код, но для протокола):**
+профилировать сам переход Iso→Flat (Chrome Performance/flamegraph, не только
+таймер снаружи) и найти, где реально уходят ~150+ мс — по всем признакам это
+не polygon-union, а стоимость самой Lit-перерисовки структурно разных
+flat/iso-шаблонов на фикстуре с 500 decor/200 устройств.
+
+## Что проверено и корректно
+
+- **Сам рефакторинг `wallBodiesPathFromGeometry`.** Извлечённая функция
+  побайтово повторяет прежнюю инлайн-логику `wallBodiesUnionPath`
+  (`d = polyclipToPathD(united.geom); return {d, depthUnits: united.depthUnits,
+  fillRule: 'evenodd'}`) — проверено чтением диффа `wall-thickness.ts` и
+  прогоном нового юнита `wallBodiesPathFromGeometry reuses the canonical union
+  without changing its flat path`, который сравнивает результат с прямым
+  `wallBodiesUnionPath` через `assert.deepEqual` — green. Тест способен
+  падать: любое расхождение в `d`/`depthUnits`/`fillRule` между переиспользуемым
+  и прямым путём завалит `deepEqual`.
+- **`_wallUnionKey()` — чистое извлечение.** Формула идентична прежней
+  инлайн-строке в `_renderWallBodies()` (`${this._space}|${this._cfgEpoch}|${...rooms.length}`)
+  — проверено чтением, оба места дают одну и ту же строку при одинаковом
+  состоянии.
+- **Отсутствие визуальной регрессии.** Полный `golden:capture` (47/47 сцен,
+  включая все 6 `isometric-*`) — 0 diff. Это ожидаемо: путь переиспользования
+  активен только при `_labsIso` (скрытый флаг), а математика union не
+  меняется, только пропускается повторный вызов.
+- **Отсутствие функциональной регрессии.** Все 127 браузерных смоков зелёные,
+  включая оба iso-специфичных (`smoke_isometric_live_touch.mjs` 21/21,
+  `smoke_isometric_contract.mjs` 14/14) и весь остальной набор (Glow, sun,
+  openings, tray, editors, backup и т.д. — ничего в этом коммите их не
+  касается, но полный повтор исключает побочный эффект).
+- **Трейлеры и процесс.** `Issue: #89`, `User-Visible: no` — верно (Labs-скрытая
+  фича, публичный changelog не требуется и не тронут). `process-gate.mjs
+  --issues` — green.
+- **Отсутствие влияния на geometry/инвалидацию кэша.** Автор пишет «Геометрия,
+  ключи инвалидизации и fallback-семантика не менялись» — подтверждаю чтением:
+  `_isoSource().key` (fingerprint для `_isoGeometryCache`) не менялся; новый
+  `wallKey` использует ту же формулу, что уже была единственным ключом
+  `_wallUnionCache` до этого коммита, только вынесенную в метод.
+- **Малый наблюдаемый риск с порядком extra-тел (не блокирует, см. ниже
+  почему).** `_isoSource().build()` берёт `extras` через отдельный вызов
+  `physicalBodies(space, cellCm, gridPitch)` (порядок: partitions → drafts →
+  columns), тогда как `_renderWallBodies()` использует мемоизированный
+  `_physicalBodiesR()` (порядок: drafts → partitions → columns). После этого
+  коммита один и тот же кэш `_wallUnionCache` может быть заполнен результатом,
+  посчитанным с любым из двух порядков — раньше он всегда шёл только через
+  `_physicalBodiesR()`. Union — коммутативная операция над одним и тем же
+  набором тел, `wallBodiesGeometry` считает `maxDepth` через `Math.max` по
+  всем телам независимо от порядка, а `golden:capture` не показал diff ни на
+  одной из 6 iso-сцен и ни на одной из существующих 41 — считаю это Low,
+  фиксирую с записью, не завожу отдельный issue: наблюдаемого расхождения нет,
+  риск чисто теоретический (устойчивость boolean-clip к порядку слияния в
+  вырожденных случаях), и я не смог построить конкретный сценарий отказа.
+
+## Чего не проверял
+
+- **Exact-SHA Full Performance сам по себе** — не CI-раннер, недоступен
+  локально; вместо него — три независимых локальных метода выше, которые я
+  считаю **достаточным** основанием для красного вердикта, но не заменой
+  официального гейта. Финальное число может отличаться от локального в
+  абсолютных мс (другое железо), но не в качественном выводе «фикс не
+  устраняет причину».
+- **Первопричину ~150–180 мс в Iso→Flat** — не профилировал flamegraph'ом
+  внутри браузера (Chrome DevTools Performance API недоступен из headless
+  `page.evaluate` в использованном мной harness без дополнительной настройки
+  трассировки); указываю наиболее вероятную гипотезу (Lit-перерисовка
+  структурно разных шаблонов), но не подтверждаю её как единственную причину.
+- **HA-harness backend-тесты** (`test_ha_*.py`) — не запускал: диапазон не
+  трогает `custom_components/**/*.py`, гейт пуст по построению.
+- **Safari/WebKit и Firefox** — вне скоупа Stage 1 (ADR), тестировал только
+  Chromium.
+
+## Вердикт
+
+**Красный · цикл r4/4 · High: 1 · Medium: 0 → #… (нет новых issue: находка H1 —
+возврат в рамках того же issue, не отдельный дефект).**
+
+Единственная блокирующая находка — H1: три независимых локальных измерения
+(браузерный round-trip A/B на двух worktree одного и того же кода до/после
+фикса, раздельные направления перехода, изолированный вычислительный
+микробенчмарк вне браузера) сходятся на том, что заявленное исправление
+экономит ≈10 мс на характерном для профиля объёме геометрии, тогда как
+prerelease-гейт требует закрыть разрыв примерно в 61 мс. Остальной диапазон
+чист: рефакторинг сам по себе корректен и покрыт тестом, который умеет падать,
+регрессий в 127/127 смоках и 47/47 golden-сцен нет, трейлеры и процесс-гейт
+зелёные.
+
+Это **r4/4** — четвёртый и последний разрешённый цикл код-ревью для этого
+issue (PROCESS.md §4). Пятого захода нет: по правилам процесса задача уходит
+владельцу на разбор — разделить, отклонить эту оптимизацию как отдельный шаг
+(например, вынести Stage 1 public rollout дальше, если сама по себе
+Labs-функция не обязана укладываться в этот бюджет ещё на этой неделе) или
+арбитраж. Я как ревьюер не имею права предлагать пятый цикл под видом
+«доработки» — это прямо запрещено §12.