From 7b59feec5e50a404fbc9216675b614249dfed1a3 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Thu, 13 Aug 2026 15:17:53 +0000 Subject: [PATCH] docs: review document for #89 Issue: #89 User-Visible: no --- docs/reviews/CODE-REVIEW-89-r4.md | 268 ++++++++++++++++++++++++++++++ 1 file changed, 268 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-89-r4.md diff --git a/docs/reviews/CODE-REVIEW-89-r4.md b/docs/reviews/CODE-REVIEW-89-r4.md new file mode 100644 index 00000000..ca205372 --- /dev/null +++ b/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.