Files
houseplan-card/docs/reviews/CODE-REVIEW-582-r2.md
2026-09-15 10:25:24 +00:00

34 KiB
Raw Permalink Blame History

CODE-REVIEW-582-r2

Issue: #582 · этап: code · заход r2 · блокирующих циклов израсходовано 2 из 4 · материал: 226f323073afd79a1eef9e2e5979684c9ae628f7 (origin/dev merge-base = 4208ca81fe3516200a5bd80c1221597492d8fa7b, дерево материала = dca5f4042deec2196302371f3ba4ffd6ab59c242).

Скоуп

Красный r1 (CODE-REVIEW-582-r1.md, материал 0a6592f4) нашёл два High: AC4 (перф-свидетель smoke_daycycle_raster красный 8/8, plan-svg продавливался в отдельный composited-слой из-за "Overlaps other composited content") и AC6 (все 4 day-cycle golden-сцены different, ~3.3–3.5% пикселей). Между r1 и r2 ветку дважды двигали: «Повторный хендофф» переписал сам механизм контура, затем ребейз на dev (конфликт с #581, материал стал 2a8b5f1d), затем красный Validate с мутантами на 2a8b5f1d (ревью не запускалось, «код никто не читал» — не цикл), и наконец стабилизация шумного raster-свидетеля коммитом 226f3230 — это и есть материал r2.

SHA r1 (0a6592f4112f) и его дерево (9f4d95168fdd) не резолвятся в этом чекауте — git cat-file -t возвращает «could not get object info», в git ls-remote origin его тоже нет. Это ожидаемо по PROCESS.md §2.10 (ребейз осиротил коммит, находкой не считается) — данные о r1 взяты не из живого диффа, а из текста docs/reviews/CODE-REVIEW-582-r1.md (коммит 728c564e, который остался в истории, так как он был закоммичен уже после материала r1) плюс из текущего кода. Раз architecture между r1 и r2 поменялась содержательно (не «одна строка в фикстуре», а новый conditional-механизм переключения слоя), разбор ниже — полный по AC1–AC7, а не только по находкам r1 (§2.9: «дельта не локальна» — сменился сам механизм, которым AC1–AC6 доказываются).

Материал и что сделано в коде (дельта r1→r2)

  • src/houseplan-card.ts: новое поле _safeDayCycleOutline и метод _activateSafeDayCycleOutline() (строка ~2068), вызываемый из трёх точек входа жеста — двухпальцевый pinch (_pinchStart+_pointers.size>=2), обнаружение _panLock==='pan' (мышь и палец через общий pointer-путь) и multitouch-старт pinch. При активации кладёт класс hp-safe-daycycle-outline на _stageEl напрямую (минуя цикл Lit — сохраняет compositor lifecycle #579) и персистентно на весь срок жизни инстанса через render().
  • Новый <svg class="hp-paper-outline-svg"> теперь рендерится ВСЕГДА при активном day-cycle (не только после жеста), но visibility: hidden до появления .hp-safe-daycycle-outline — то есть до первого движения камеры карточка сохраняет байт-в-байт прежнюю (r1-довоенную, #532) однослойную композицию: .hp-paperg держит filter+will-change:filter напрямую, а скрытый сосед не создаёт собственного promoted-слоя (у него нет фильтра до активации).
  • После активации: .hp-paperg получает filter:none; will-change:auto (перестаёт быть большим координатным слоем), фильтр и will-change:filter переезжают на .hp-paper-outline-svg (stage-размерный корневой <svg>), а .plan-svg получает явный will-change: transform — это и есть исправление находки №1 r1: явная причина промоушена не даёт Chromium придумать implicit "Overlaps other composited content" для всего плана.
  • src/render/paper-scene.ts — новый общий renderPaperShapes() (вынесен из r1, без изменений сигнатуры), используется в houseplan-card.ts и space-render.ts.
  • src/space-render.ts/src/space-card.ts — не получили hp-paper-outline-svg/ hp-safe-daycycle-outline вообще (в r1 они были, симметрично основной карточке). Это разбирается как находка №1 ниже.
  • demo/smoke_daycycle_layer_budget.mjs (новый в r1, здесь без изменений сигнатуры проверок) — свидетель AC1–AC3, теперь дополнительно проверяет, что до жеста контур visibility:hidden, а после — .plan-svg получает will-change:transform явным слоем.
  • demo/smoke_daycycle_raster.mjs — переписан в r2 (комментарий issue «Исправление CI-свидетеля»): три чередующиеся замкнутые static/day-cycle пары, медиана парных отношений вместо одиночного замера — устраняет ложный красный на шумном раннере, порог 2.0 не менялся.
  • scripts/mutation-registry.mjs — новый мутант daycycle-outline-promoted-on-inner-paper (возвращает will-change:filter на .hp-paperg в safe-режиме), старый daycycle-outline-not-promoted адаптирован (снимает will-change:filter с общего блока).
  • Документация (ARCHITECTURE.md, SUN.md, TOUCH-SUPPORT.md, TESTING.md) переписана под новый двухфазный механизм и прямо признаёт, что статическая карточка в фолбэк не входит («the non-interactive static space card keeps its historical inner outline because it has no camera gesture (#582)» — docs/SUN.md).
  • Changelog (docs/CHANGELOG.md/.ru.md) не менялся в этой дельте — запись сделана ещё в a68e4c6b (User-Visible: yes) и по-прежнему точно описывает финальное поведение основной карточки (перепроверено golden/smoke-прогонами ниже).

Как проверялось

Дешёвые гейты подтверждены зелёным Validate на точном материале (https://github.com/Matysh/houseplan-card/actions/runs/34954286064) — предполёт, «Фронтенд: типы/юниты/бандл», мутанты по диффу: success. Тяжёлые джобы в этом прогоне не гейтили пуш (обычный push, не Release:/PR/full=true) — именно их r1 не хватило, поэтому здесь они прогнаны мной лично.

Гейт Результат Почему прогнан/не прогнан
npm run bundle:sync (typecheck+build+rollup+bundle-tree) зелёный нужен как основа для браузерных смоков; дёшево
npm run bundle:budget зелёный, 291058 Б / потолок 291700±2000, запас 10008 Б (предупреждение о низком общем запасе — преэкзистентный долг #367/#474, не находка этой задачи) обязателен после билда
node scripts/check-docs.mjs зелёный, 7 файлов, 12 внешних ссылок diff трогает src/** — обязателен всегда
node scripts/process-gate.mjs --issues зелёный, 5 коммитов, 0 предупреждений офлайн-проверка трейлеров/веток
node demo/smoke_daycycle_layer_budget.mjs (AC1/AC2/AC3) зелёный, все 12 заявленных инвариантов true, 12/12 presented-кадров nearWhiteRatio:0 прямое совпадение — это и есть свидетель находок r1
node demo/smoke_daycycle_raster.mjs (AC4) зелёный, медиана парных отношений 1.55 (1.39/1.55/1.69) при потолке 2.0 прямое совпадение — это и есть красный тест r1
npm run golden:verify (AC6, полная матрица 172 сцены) зелёный, 172/172 passed, все 4 day-cycle-{dawn,day,dusk,night}-dark в их числе diff меняет рендер; это и есть красная проверка r1
node demo/smoke_bg_color.mjs зелёный (все проверки, включая staticCardLayersStayOrdered) зарегистрированная связь renderPaperShapes
node demo/smoke_live_pan_coverage.mjs зелёный зарегистрированная связь renderPaperShapes
node demo/smoke_pan_any_zoom.mjs зелёный (pinch/pan/kiosk-swipe инварианты) прямое совпадение по _panLock, тот же код-путь, что получил новый вызов _activateSafeDayCycleOutline()
node demo/smoke_isometric_contract.mjs зелёный прямое совпадение — plan-svg класс стал безусловным, iso-обёртка контура повторяет isoFloorMatrixCss()
node scripts/mutation-gate.mjs --id=daycycle-outline-promoted-on-inner-paper зелёный: чистый прогон ok, мутант поймал (1 из 1) новый мутант AC2, дорогой гейт — обязателен по §2.7 (защита живёт в продуктовом коде)
node scripts/mutation-gate.mjs --id=daycycle-outline-not-promoted зелёный: чистый прогон ok, мутант поймал (1 из 1) существующий мутант r1, адаптирован под новый общий блок — перепроверен, что не сломался
npx tsc --noEmit, npm test, npm run build (сверка копий) не гонял отдельно — подтверждены зелёным Validate на этом же SHA дешёвые гейты, разрешено ссылаться на зелёный прогон (npm run build я всё равно выполнил как часть bundle:sync)
python -m pytest tests_backend, HACS, Hassfest, geometry-parity не гонял diff не трогает custom_components/**/*.py, манифесты, layout/marker.space/толщину стен — только SVG-рендер и стили
node scripts/model-invariants.mjs не гонял diff не меняет геометрическую модель/рёбра/толщину — только композитинг и разметку SVG
Остальные 40+ «прямое совпадение» смоков из smoke-select.mjs (_mode, _modeTransitionBusy, _decorTool, _stageEl, _booting и т.п.) не гонял, кроме двух выше это артефакт одной длинной изменённой JSX-подобной строки шаблона (<div class="stage ...">), в которой лежат десятки не изменившихся по смыслу свойств — слабая связь по PROCESS.md §8; прогнал два наиболее правдоподобных (smoke_pan_any_zoom, smoke_isometric_contract) точечно
performance_smoke (полный, вне smoke_daycycle_raster) не гонял AC4 уже проверен целевым, более дешёвым свидетелем; полный профиль не добавляет решающей информации на этом раунде

Закрытие раунда r1

Находка r1 Чем закрыта Где это видно
High №1 — AC4 не держится: plan-svg продавлен в отдельный слой ("Overlaps other composited content"), raster ratio 2.16–2.50 против потолка 2.0 Явный will-change: transform на .plan-svg в safe-режиме убирает implicit-промоушен; контур скрыт до жеста, поэтому до-жестовый idle-рендер не платит вообще src/styles/plan.styles.ts:111-113 (.stage.daycycle.hp-safe-daycycle-outline .plan-svg { will-change: transform; }); мой прогон node demo/smoke_daycycle_raster.mjs — медиана 1.55 при потолке 2.0
High №2 — AC6 не проходит: все 4 day-cycle golden different, ~3.3–3.5% пикселей Контур не рендерится видимо (visibility:hidden) и не меняет .hp-paperg до первого жеста — golden снимаются на idle-карточке, которая теперь байт-в-байт прежняя композиция src/styles/plan.styles.ts:264-270 (.hp-paper-outline-svg{visibility:hidden} / .stage.hp-safe-daycycle-outline .hp-paper-outline-svg{visibility:visible}); мой прогон npm run golden:verify — 172/172 passed, все 4 day-cycle сцены включены
Low №3 — комментарий про «the external day-cycle outline... is composited» на .hp-paperg, хотя фильтр туда больше не крепится Комментарий переписан под новую архитектуру src/houseplan-card.ts, блок над renderPaperShapes(paperShapes): «One <g> keeps the visible sheet and filtered silhouette free of room seams.» — прочитано, соответствует коду

Находки

Находка №1 (High) — Контракт п.6 не выполнен: houseplan-space-card не получил ограниченный экранными размерами механизм

Файлы: src/styles/plan.styles.ts:93-95 (правило .hp-static-stage.daycycle .hp-paperg), src/space-render.ts, src/space-card.ts.

ТЗ (раздел «Контракт поведения», пункт 6, зелёное спек-ревью r1, без замечаний по этому пункту):

Тот же ограниченный экранными размерами механизм применяется в houseplan-space-card, чтобы статическая карточка не сохраняла скрытый координатно-зависимый риск. Внешний вид и отсутствие интерактивных жестов этой карточки не меняются.

Текущий код .hp-static-stage.daycycle .hp-paperg не тронут дельтой r1→r2 вообще — та же самая строка will-change: filter на координатно-большой .hp-paperg, что и до всей задачи #582/#532. Ни hp-paper-outline-svg, ни hp-safe-daycycle-outline в space-render.ts/ space-card.ts не существует (в r1 они были — см. «Что проверено и корректно» r1: «hp-static-stage получил аналогичное правило»; в r2 это убрано полностью).

Это не домысел ревьюера — это задокументированное и осознанное решение автора. Прямая цитата из диффа docs/SUN.md (r2): «the non-interactive static space card keeps its historical inner outline because it has no camera gesture (#582)»; из docs/ARCHITECTURE.md (r2): «the static space card, which has no camera gesture, never enters the fallback». Тест test/paper-scene-contract.test.mjs кодифицирует это же самое как требование: assert.doesNotMatch(staticRender, /class="hp-paper-outline-svg"/) и assert.doesNotMatch(staticCard, /\.hp-static-stage \.hp-paper-outline-svg/) — то есть попытка исправить это в будущем сломает собственный тест задачи.

Почему это High, а не молчаливое наследование. Формулировка контракта явно и без условий: «скрытый» риск назван так именно потому, что он не требует жеста — houseplan-space-card рендерит тот же canonicalWallGeometry.paperD/paperRoomShapes(space.rooms) из того же пространства с тем же cell_cm, то есть ровно тот же 4700×4200-координатный диапазон при cell_cm:1, что и основная карточка (тот самый пример из тела issue). houseplan-space-card — read-only карточка для встраивания (docs/ARCHITECTURE.md:1577), доступная в тех же дашбордах HA Companion/kiosk, где и воспроизведён исходный дефект (docs/SCOPE.md: View — продукт для двух из трёх персон, touch/View — release-blocking по docs/TOUCH-SUPPORT.md). Ни один из AC1–AC9 явно не тестирует компоситорную безопасность статической карточки (AC5 проверяет только визуальную целостность контура и отсутствие лишней outline-сцены у статичного ФОНА/редакторов/ изометрии — это другой параметр, не про houseplan-space-card; AC7 проверяет только переиспользование общего рендерера форм, не компоситинг) — то есть пробел в контракте образовался и не пойман собственным тест-планом задачи.

Технически это не тот же самый регресс, что нашёл r1 (никакого overlap-промоушена plan-svg в static-карточке нет, потому что там нет второго <svg>-соседа вообще) — риск ровно тот, ради устранения которого пункт 6 был написан: постоянный координатно-большой will-change:filter. Раз пункт 6 сформулирован как безусловное требование, принятое зелёным спек-ревью, и раз реализация сознательно его не выполняет без вынесения вопроса владельцу (ТЗ §7.1 требует эскалации продуктовых развилок, а не тихого решения в одиночку) — блокирует.

Проверено чтением (селекторы CSS, вызовы _activateSafeDayCycleOutline, текст документации и теста), не браузерным прогоном: демонстрация самого белого-тайла на статической карточке потребовала бы отдельного HA Companion WebView стенда, которого нет ни у одного участника этого раунда; риск подтверждён структурно (тот же координатный размер, тот же непрерывный will-change:filter), а не количественно замерен.

Находка №2 (Medium, в скоупе) — Анимированные переходы камеры (double-tap, «Вписать всё», клик по комнате, колесо/кнопки zoom) не активируют safe-режим

Файлы: src/houseplan-card.ts — _onWheel (6727), _stepZoom (6744), _resetZoom/_fitAll (6764/6396, включая reason:'double-tap' из _doubleFit.pointerUp на строке 7044), переход по клику на комнату (reason:'room', строка 6381) — все они вызывают _startCameraTransition(...), которая не вызывает _activateSafeDayCycleOutline() нигде.

_activateSafeDayCycleOutline() вызывается только из трёх точек прямого pointer-move — двухпальцевый pinch, _panLock==='pan' (drag) и multitouch-старт. Контракт п.3 ТЗ говорит «во время всей pinch/pan-сессии», и AC1–AC4/AC9 буквально описывают только pinch — так что это не прямое нарушение контракта (в отличие от находки №1), но double-tap-to-fit и room-fit — такие же многокадровые анимированные изменения viewBox (CAMERA_FIT_MS), достижимые касанием на планшете-киоске (тот же класс устройства, где воспроизведён #582), и физический механизм риска (большой координатный will-change:filter на .hp-paperg во время многокадрового изменения viewBox) не привязан семантически именно к жесту «два пальца» — он привязан к тому, что слой вообще движется на экране несколько кадров подряд. Пользователь, который на планшете только дважды тапает «показать всё» или нажимает кнопку зума и никогда не щипает/не тащит план одним пальцем, весь сеанс сохраняет исходную #532-риск-топологию.

Не проверено запуском (нет прямого свидетеля double-tap/room-fit в новом смоке — smoke_daycycle_layer_budget.mjs покрывает только явный touch-pinch путь); проверено чтением всех точек входа _startCameraTransition. В скоупе задачи (тот же файл, тот же механизм) — чинится добавлением вызова _activateSafeDayCycleOutline() в _startCameraTransition (или во все её вызывающие точки) либо явным решением сузить контракт п.3 до буквально pinch/pan с вынесением вопроса владельцу, почему double-tap/room-fit исключены.

Что проверено и корректно

  • AC1–AC3 (layer budget, mutant AC2, presented-frame screencast) — подтверждены свежим запуском smoke_daycycle_layer_budget.mjs: 0 слоёв больше 4096, суммарная площадь экранного порядка, 12/12 presented-кадров без белых пикселей, .hp-paper-outline-svg visibility:hidden→visible ровно при активации hp-safe-daycycle-outline.
  • AC4 — перепроверен на новом, медианно-стабилизированном свидетеле: 1.55 при потолке 2.0 (мой собственный прогон, три чередующиеся замкнутые пары); мутант daycycle-outline-not-promoted ловит регрессию (node scripts/mutation-gate.mjs --id=... — поймано 1 из 1).
  • AC6 — все 172 сцены golden-матрицы, включая 4 day-cycle, passed на каноничном Linux (мой собственный прогон npm run golden:verify).
  • AC2 — новый мутант daycycle-outline-promoted-on-inner-paper зарегистрирован на верном guard-файле и реально ловит возврат will-change:filter на .hp-paperg в safe-режиме (мой запуск, поймано 1 из 1).
  • Единый живой viewport (#579). data-hp-live-viewbox стоит на .hp-paper-outline-svg тем же значением, что на plan-svg; активация safe-режима идёт напрямую через classList.add на _stageEl, минуя ожидание Lit re-render — сохраняет контракт п.4 (бюджетированные обновления viewBox без полного re-render на каждый pointer-кадр). Проверено чтением плюс smoke_daycycle_layer_budget (cameraActivatesSafeFallback, settledPlanLayerIsExplicitAndBounded).
  • Изометрия. plan-svg стал безусловным классом ещё в r1 (принято тогда же, не новая находка); smoke_isometric_contract.mjs зелёный на этом материале.
  • Мутация/тест-план. Оба мутанта #582 (старый и новый) реально ловятся своими guard-смоками, не только заявлены.
  • Changelog/трейлеры. Issue: #582+User-Visible: yes на a68e4c6b, оба changelog правлены в нём же и по-прежнему точно описывают финальное поведение основной карточки (перепроверено — ни pinch-путь, ни визуальный контур с r1 не изменились содержательно для пользователя, описание не вводит в заблуждение относительно находки №1, поскольку не упоминает static-карточку явно). Коммиты 2a8b5f1d/226f3230 — User-Visible: no, соответствует правилу (продуктовое поведение основной карточки не меняется относительно уже описанного, правится только устойчивость реализации/свидетеля).
  • Документация. ARCHITECTURE.md/SUN.md/TOUCH-SUPPORT.md/TESTING.md построчно соответствуют новому коду, включая честное признание про статическую карточку (которое и стало находкой №1 — документация не скрывает пробел, она его называет).
  • node scripts/check-docs.mjs, node scripts/process-gate.mjs --issues, npm run bundle:budget — зелёные на материале.

Унаследовано из r1

Документ: docs/reviews/CODE-REVIEW-582-r1.md (коммит 728c564e), материал r1 — 0a6592f4112fd7ceef68cbd3be7c44eeb8a7ef3e (SHA осиротел после ребейза, не резолвится в этом чекауте — по PROCESS.md §2.10 это не находка). Принято без повторной проверки, так как дельта r1→r2 этого не касается:

  • AC7 (unit/structure, переиспользуемый рендерер бумаги). renderPaperShapes() — тот же API, что в r1, используется в обеих поверхностях без изменений сигнатуры; существующее покрытие «пустого и составного плана» в test/logic.test.mjs/test/wall-thickness.test.mjs (найдено grep, не запускалось повторно отдельно от npm test, зелёного на Validate).
  • Спек-ревью r1 (SPEC-REVIEW-582-r1.md, зелёный, High:0/Medium:0) — ТЗ с тех пор не менялось (тело issue не редактировалось после спек-ревью; проверено визуально, хэш якоря спек-ревью не пересчитывался в этом раунде за неимением инструмента конвейера в среде ревью — риск низкий, раздел «Контракт поведения» текстуально совпадает с тем, что цитирует r1).
  • AC8 (quality gates состав) — перечень необходимых гейтов не изменился между раундами.
  • AC9 (полевая приёмка) — вне этапа код-ревью по определению самого AC, не переоценивается.
  • i18n/миграция/откат/риски раздела ТЗ — не затронуты дельтой, содержательно не изменились.

Чего не проверял и почему

  • python -m pytest tests_backend, HACS, Hassfest, geometry-parity — diff не трогает custom_components/**/*.py, манифесты, layout/marker.space/толщину стен.
  • node scripts/model-invariants.mjs — diff не меняет геометрическую модель/рёбра решётки/ записи толщины, только SVG-композитинг и разметку.
  • Полная матрица demo/smoke_*.mjs (250 файлов) — прогнаны 6 (два «зарегистрированная связь», два прямых по renderPaperShapes-цепочке и два точечно выбранных из 44 «прямых» слабых совпадений). Остальные 42 «прямых» совпадения — артефакт одной длинной изменённой строки шаблона со множеством не изменившихся по смыслу свойств (_mode, _booting, _modeTransitionBusy, _decorTool, _stageEl и т.п.); не прогнаны сознательно, см. таблицу выше.
  • Полный performance_smoke/профили вне smoke_daycycle_raster.mjs — AC4 уже подтверждён на целевом, более дешёвом свидетеле.
  • npx tsc --noEmit/npm test/npm run build отдельно от bundle:sync — приняты по зелёному Validate на этом же материале (https://github.com/Matysh/houseplan-card/actions/runs/34954286064).
  • Реальный HA Companion WebView на живом устройстве (AC9) — вне этапа код-ревью; это же ограничение не позволило количественно подтвердить находку №1 на статической карточке (подтверждена структурно, не количественно).

Вывод

Оба High r1 закрыты предметно: AC4 (raster ratio 1.55 против потолка 2.0, воспроизведено самостоятельно) и AC6 (172/172 golden, включая все 4 day-cycle сцены, воспроизведено самостоятельно) — новый двухфазный механизм (hp-safe-daycycle-outline, активируемый только жестом) решает находки r1 без регрессии производительности контура из #532. Но при этом дельта r1→r2 предметно убрала защиту статической карточки, которую r1 реализовывал (там она была симметрична основной), и пункт 6 контракта ТЗ («тот же ограниченный экранными размерами механизм применяется в houseplan-space-card») сейчас не выполнен — задокументировано самим автором как осознанное решение, не вынесенное владельцу как продуктовая развилка. Это High, в скоупе задачи, чинится в этой же ветке. Дополнительно найден Medium: анимированные (не pinch/pan) переходы камеры не активируют safe-режим, что оставляет часть touch-путей (double-tap-to-fit, room-fit, кнопки/колесо зума) в исходной риск-топологии — тоже в скоупе, тоже чинится здесь. Возврат автору.


Материал раунда

  • Ветка: issue/582-webview-large-filter-layers, HEAD 226f323073afd79a1eef9e2e5979684c9ae628f7.
  • Дерево материала: dca5f4042deec2196302371f3ba4ffd6ab59c242.
  • origin/dev на момент ревью: 4208ca81fe3516200a5bd80c1221597492d8fa7b (merge-base с HEAD).
  • Предыдущий документ: docs/reviews/CODE-REVIEW-582-r1.md (материал 0a6592f4112f, вердикт red, High 2 — SHA осиротел после ребейза, не резолвится в этом чекауте, см. §2.10).

Материал раунда

  • Ветка: issue/582-webview-large-filter-layers, коммит 226f323073af — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
  • Дерево материала: dca5f4042deec2196302371f3ba4ffd6ab59c242
    git log --all --format='%H %T' | grep dca5f4042dee
    
  • Тело issue: 4c1e1ecfe8ce205f3cfb3c44eb6028a35d0bb4358323e05ba60c6fd85b3b40ba
  • Вердикт конвейера: red · High 1