From 0cbc7aff7aaa4ef41b3b3a9e1ea1973fcb2f8c90 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Mon, 14 Sep 2026 19:01:08 +0000 Subject: [PATCH] docs: review document for #579 Issue: #579 User-Visible: no --- docs/reviews/SPEC-REVIEW-579-r1.md | 255 +++++++++++++++++++++++++++++ 1 file changed, 255 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-579-r1.md diff --git a/docs/reviews/SPEC-REVIEW-579-r1.md b/docs/reviews/SPEC-REVIEW-579-r1.md new file mode 100644 index 00000000..11ff8d5a --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-579-r1.md @@ -0,0 +1,255 @@ +# SPEC-REVIEW-579-r1 + +- Issue: #579 — «HA Companion: при pinch-zoom стены и пол мигают белым или прозрачным» +- Этап: spec (PROCESS.md §2.4), заход r1, блокирующих циклов израсходовано 0 из 4 +- Материал: тело issue #579 целиком (аналитика → `## ТЗ`, 14 разделов) плюс один + комментарий-аналитика (`S2`), метка `S4-spec-review`; предыдущих раундов + ревью в issue нет — это первый заход, разбор полный, раздела «Унаследовано» + не требуется +- Проверено на `dev` @ `856a2c6e1238d5cb735f0c4676af2f2d8edfc838` (рабочая копия + уже на этом SHA — сверено `git rev-parse HEAD` и `git log -1`); тот же SHA + назван автором в комментарии-аналитике как точка проверки + +## Скоуп ревью + +Ревью ТЗ, не кода: `src/live-viewport.ts` ещё не менялся (issue открыт, +продуктового коммита по #579 нет). Вопрос — выполнимо ли и однозначно ли ТЗ, +доказуем ли каждый AC, не выдана ли догадка за факт, верно ли выбран полный +трек, и нет ли продуктового вопроса, спрятанного в тексте под видом +технического. + +## Как проверялось + +1. Прочитан `docs/SCOPE.md`: задача чинит поломку J1 («Show the whole home and + what's happening right now») ровно в той поверхности, которую SCOPE.md + называет решающей для двух из трёх персон — «View mode is the product» + (Household members, Guests/kiosk). Новой job не открывает, рамку не + расширяет. +2. Прочитаны `AGENTS.md` и PROCESS.md §2.4, §2.9/2.10 (не применимо — r1), + §4, §7.1, §7.2. +3. Прочитано тело issue #579 целиком: аналитический блок до `## ТЗ` (причина, + почему тесты зелёные, связанные issue, диагностическая развилка, границы + исправления, покрытие будущей реализации) и сам `## ТЗ` (14 разделов). + Прочитан единственный комментарий — S2-аналитика владельца, подтверждающая + механизм на этом же SHA и явно снимающая продуктовые вопросы. +4. Прочитан `docs/TOUCH-SUPPORT.md`: pinch/pan в View и kiosk — гарантированная, + release-blocking поверхность («support convenient pan, pinch zoom and space + switching», «A touch-only failure in View is a product defect, not an + accepted limitation»). Задача лежит строго внутри этого контракта, не + пытается его расширить или сузить. +5. Прочитан `docs/CANVAS.md` §5 (Zoom and pan): «Pinch and pan remain direct + 1:1 gestures», модель zoom/pan/anchor не входит в скоуп задачи и прямо + исключена в её не-скоупе — согласовано. +6. Прочитан `docs/ARCHITECTURE.md`, раздел «Live viewport: a transform per + frame, a `viewBox` on a budget (#531, 2026-09-11)» — канонический текст + подсистемы. Он же документирует ныне действующее поведение, которое ТЗ + собирается менять: «A budget refresh or the terminal commit removes the + inline overflow together with the transform» — совпадает с описанием + дефекта в ТЗ дословно, не расходится. +7. **Технический механизм дефекта сверен построчно с кодом на названном SHA**, + а не принят на веру: + - `src/live-viewport.ts:190-205` (`paintLiveViewport`): на каждом кадре + сцена-SVG проецируется от `next.frame` к `current` через + `liveLayerProjection`; при бюджетном refresh (`needsViewBoxRefresh`, + `:93-103`, 100 мс либо сдвиг/масштаб 15 %) `next.frame` становится равен + `current` → проекция тождественна → `isIdentityLiveLayerProjection` + возвращает `true` → в `setLayerProjection` передаётся `null` + (`:192-205`), что физически снимает `transform`/`transform-origin`/ + `will-change` и (при `exposeSceneOverflow`) `overflow` (`:112-146`, + ветка `!projection`). Следующий кадр жеста снова получает нетождественную + проекцию и заново промотирует слой. Это ровно тот churn + промотирования/демотирования, который ТЗ называет причиной дефекта — + подтверждено чтением, а не предположением. + - `commitHouseplanViewport` (`:255-270`) вызывается из + `LiveInteractionRuntime.commit()` (`src/live-interaction-runtime.ts:79`), + который, в свою очередь, вызывается из `protected updated()` + (`src/houseplan-card.ts:4202-4203`) — то есть **на каждом полном Lit-цикле + обновления**, включая конец программной камеры (переход анимации меняет + реактивные `_view`/`_zoom`, что вызывает `requestUpdate` → `updated()`). + Это подтверждает контрактный пункт 4 и 9 ТЗ («тот же lifecycle для + pointerup/pointercancel/lostpointercapture/программной камеры/обычного + Lit-коммита, не по user-agent») без необходимости заводить новые + обработчики событий — он уже общий по архитектуре. + - `setLayerProjection` (`:112-146`) уже содержит write-only-on-change + защиту (`if (style.transform === text) return;`, `:142`, и аналог для + `viewBox` в `setViewBox`, `:149-151`) — то есть требование АС1/АС2/АС9 + «держать identity transform явно установленным без лишних записей на + неизменный кадр» технически совместимо с существующим примитивом: нужно + не убирать значение, а один раз записать identity-текст, дальше сравнение + строк само не даст повторных записей. + - Селектор `data-hp-live-viewbox="camera"/"floor"` (`houseplan-card.ts`, + множественные точки: iso underlay/shadows/walls/overlays, flat/iso + floor-svg, `vactrail`, `radar-ranges`) уже общий (`querySelectorAll`, без + перечисления по имени) — значит формулировка ТЗ «каждый scene-SVG с + `data-hp-live-viewbox=camera/floor`» реализуется тем же общим + механизмом, не требует адресного списка и не рискует пропустить один из + iso-слоёв. + - Риск ТЗ «изометрия имеет несколько SVG с разными camera/floor + проекциями; применение одной матрицы ко всем запрещено» — подтверждён: + в коде камера и floor действительно проецируются раздельно + (`sceneCamera`/`sceneFloor`, `:190-191`, отдельные querySelectorAll на + `:192` и `:199`), общий рефакторинг рискует их случайно смешать, и ТЗ + этот риск называет явно, а не молчит о нём. +8. Сверены названные в плане автотестов файлы/инструменты — все существуют, + ни один не выдуман: `test/live-viewport.test.mjs`, + `demo/smoke_live_pan_coverage.mjs`, `demo/screencast_visual_continuity.mjs` + (CDP screencast harness, npm-скрипт `continuity:screencast`), + `demo/smoke_editor_gestures.mjs`, `demo/smoke_smooth_zoom.mjs`, + `demo/smoke_tap_ctx.mjs`, `demo/smoke_long_press_gesture.mjs`, + `demo/smoke_isometric_live_touch.mjs`, `scripts/mutation-gate.mjs`. +9. Попытка воспроизвести приведённый в issue вывод + `node demo/smoke_live_pan_coverage.mjs` (`OK — 37/37 checks green`) — + локально упала на старте (`Failed to fetch dynamically imported module`, + таймаут `page.waitForFunction`), потому что нет собранного `dist/**` + (продуктовый код по #579 не менялся, бандл собирался в другой раз). Это + ожидаемо для стадии spec (гейты и сборка сюда не относятся, см. ниже) и не + ставит под сомнение вывод: сам механизм дефекта я подтвердил прямым чтением + исходников (п.7) — более надёжным способом, чем повторный прогон уже + процитированного зелёного смока. +10. Проверено согласование трека: `small` (PROCESS.md §5) требует одну + поверхность и отсутствие нового touch/производительного контракта — + задача одновременно трогает compositor lifecycle, кросс-браузерную + производительность (Chrome/Firefox) и touch-контракт View/kiosk; владелец + в аналитике явно пишет «лёгкий трек: нет» с обоснованием. Полный трек + выбран верно. + +Гейты (`typecheck`/`test`/`build`/`golden`) на этой стадии не прогонялись и не +нужны: продуктового кода по #579 ещё нет, предмет проверки — текст ТЗ, а не +бинарник (см. п.9 выше — попытка была сделана ровно для проверки +воспроизводимости цитаты из issue, а не как обязательный гейт стадии). + +## Находки + +Ни одной High или Medium находки. + +- **Проверено и не является находкой: возможная догадка о причине.** Аналитика + (до `## ТЗ`) прямо помечает локализацию как «подтверждённая, но не доказанный + конкретный фикс» и требует A/B-развилку из 5 вариантов до выбора реализации. + Раздел «Принятые предположения» отдельно и честно выделяет «причина — churn + promotion/demotion, а не конкретный фильтр/геометрия» как предположение, + которое можно поменять на ревью. Я сам подтвердил именно этот механизм прямым + чтением кода (см. «Как проверялось», п.7) — то есть то, что в ТЗ помечено как + предположение, на самом деле уже доказано чтением; это не ухудшает ТЗ (лишняя + осторожность автора не вредит), поэтому не является дефектом текста. +- **Проверено и не является находкой: чем закрывается AC10 без физического + устройства.** ТЗ прямо возлагает финальную полевую проверку на владельца + перед закрытием beta candidate и разрешает честно отметить «невыполненная + полевая приёмка» вместо подмены Chromium-скриншотом. Это не новый прецедент: + `docs/reviews/CODE-REVIEW-544-r1.md` («физическое устройство — не локальное + доказательство») ровно так же выносит полевую проверку за пределы + автоматического доказательства того же самого подкласса дефекта (Companion + WebView). Формулировка AC10 не создаёт скрытого продуктового вопроса и не + блокирует «Готово к разработке» — критерий доказуем в том виде, в каком + сформулирован. +- **Проверено и не является находкой: смешение технического и продуктового + вопроса.** Раздел «Продуктовые вопросы владельцу: нет» обоснован — + пользовательский контракт («ни один показанный кадр не теряет пол/стены») + уже зафиксирован в `docs/TOUCH-SUPPORT.md` как release-blocking для View, а + выбор конкретного compositor-механизма — техническое решение авторов + (§7.1: «где хранится состояние… агенты решают сами»), корректно вынесенное + в раздел «Технический механизм требует A/B до реализации», а не владельцу. + +## Проверка обязательных разделов (PROCESS.md §7.1) + +Все обязательные разделы на месте и в правильном порядке: сценарий · что +человек увидит до и после · проблема · скоуп и не-скоуп · контракт поведения +(9 пунктов) · UX · модель данных и миграция (эфемерное состояние +`LiveViewportState`, явно «новых полей/ключей/миграций нет») · i18n (явно +«новых строк нет») · критерии приёмки AC1–AC10, каждый со способом +доказательства · план автотестов · риски · откат · release-артефакты. Плюс +полезные необязательные разделы: аналитика по коду, диагностическая развилка, +предварительные границы, требуемое покрытие будущей реализации, приоритет и +трек, принятые предположения. + +- **Сценарий/что человек увидит** — персона-центричны («пользователь Home + Assistant… в Companion на телефоне»), без терминов реализации; «промежуточный + кадр», «pinch», «zoom», «View», «kiosk» — уже канонические термины подсистемы + (`CANVAS.md`, `TOUCH-SUPPORT.md`, `USER-GUIDE.ru.md`), не изобретены задачей. +- **AC однозначны и проверяемы.** Каждый из AC1–AC10 называет способ + доказательства (unit/browser smoke/screencast/mutation witness/полевая + проверка), большинство — с отрицательным свидетелем (AC1 и AC6 явно требуют + красного mutation witness; AC10 явно требует красного текущего поведения на + реальном устройстве до фикса — это условие «тест умеет падать» будущего + код-ревью, зафиксированное уже в самом ТЗ). +- **Скоуп/не-скоуп** согласован с `SCOPE.md` (J1) и `TOUCH-SUPPORT.md` + (View/kiosk — гарантированная поверхность); не расползается: явно исключены + геометрия плана, диапазон/якорь/инерция zoom, UA-sniffing, отключение + эффектов, рефакторинг Companion/Chromium, публикация изометрии. +- **Контракт поведения** (9 пунктов) внутренне непротиворечив и совместим с + существующим кодом: п.7 («частота записей viewBox не возрастает») не + конфликтует с п.2/п.3 («identity transform остаётся явно установленным») — + это разные атрибуты (`viewBox` vs `style.transform`), и существующий + write-on-change примитив (`:142`, `:149-151`) их не смешивает. +- **Принятые предположения** оформлены отдельным блоком «можно менять на + ревью» с явным обоснованием каждого пункта — ровно то разделение + «продуктовое решает владелец, техническое — авторы», которого требует §7.1. + Открытых продуктовых вопросов, замаскированных под технические, не найдено. +- **Риски** называют конкретные точки провала предположения (GPU-память + постоянно промотированного слоя; WebView может мигать и внутри уже + стабильного layer — с названным следующим техническим вариантом на этот + случай; identity transform может дать другой settled raster — с названным + контролем через golden/cleanup-unit; смешение iso camera/floor проекций) — + не декоративные, а операционные. +- **Откат** — ревертом единственного продуктового коммита, без данных/миграции; + единственная оговорка (возврат к покадровому `viewBox` как временная мера при + доказанной GPU-регрессии) явно помечена как повторное открытие исходного + дефекта, не как тихий компромисс. + +## Что проверено и корректно + +- Технический механизм дефекта (churn промотирования/демотирования scene-SVG + на каждом budget refresh) подтверждён построчным чтением `live-viewport.ts` + и `houseplan-card.ts` на названном SHA, совпадает и с текстом ТЗ, и с + действующим описанием в `docs/ARCHITECTURE.md`. +- Существующая архитектура (write-on-change примитивы, общий селектор + `data-hp-live-viewbox`, generic `updated()`-коммит) технически допускает + предложенный фикс без новых обработчиков событий или специальных случаев — + «Техническая реализация» не описывает невозможную или противоречивую схему. +- Ни одно техническое утверждение ТЗ, не помеченное как предположение, не + разошлось с кодом или каноническими документами при проверке. +- Названные в плане автотестов файлы и npm-скрипты существуют, инфраструктура + для AC1–AC9 реальна, а не гипотетична. +- Продуктовых вопросов владельцу нет обоснованно: контракт «ни один показанный + кадр не теряет пол/стены» уже зафиксирован `TOUCH-SUPPORT.md` как + release-blocking, новых продуктовых решений задача не вводит. + +## Чего не проверял + +- Гейты `typecheck`/`test`/`build`/`golden` — не прогонялись: продуктового + кода ещё нет, предмет этапа — текст ТЗ, а не бинарник. +- Полный `node demo/smoke_live_pan_coverage.mjs` — попытка запуска сделана + только для проверки воспроизводимости цитаты из issue (см. п.9), упала на + отсутствии собранного `dist/**`; пересборка бандла на этой стадии не + требуется и не выполнялась. +- Реализация будущего mutation witness и содержимое расширенного + `test/live-viewport.test.mjs`/`smoke_live_pan_coverage.mjs` — не существуют + на этой стадии, их проверит код-ревью. +- Ручного/физического Companion-теста не проводилось и не требовалось: ТЗ + прямо и корректно относит его к AC10 как отдельный релизный гейт перед + закрытием issue, а не как доказательство этого раунда. +- Perfomance-профиль большого плана (AC9) — назван «перед бетой», вне скоупа + ревью ТЗ. + +## Вердикт + +Зелёный. High — 0, Medium — 0. Все обязательные разделы §7.1 присутствуют, +каждый AC однозначен, проверяем и снабжён доказательством и (где применимо) +отрицательным свидетелем. Технический механизм дефекта подтверждён прямым +чтением кода на названном SHA, а не принят на веру. Единственное +предположение, обозначенное как таковое, на практике уже доказано чтением +кода. Продуктовых вопросов владельцу нет обоснованно. Задача может переходить +в «Готово к разработке». + +--- + + + +## Материал раунда + +- Ветка: `issue/579-webview-pinch-compositor`, коммит `856a2c6e1238` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `35e12ab83a88ef61b8284ef386341e4e71f40b89` + ``` + git log --all --format='%H %T' | grep 35e12ab83a88 + ``` +- Тело issue: `4545e4010a0a0d13588d41c73912f543b83f5091653de15127390b77630498a0` +- Вердикт конвейера: `green` · High 0