mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-30 11:49:16 +00:00
@@ -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, а не принят на веру. Единственное
|
||||
предположение, обозначенное как таковое, на практике уже доказано чтением
|
||||
кода. Продуктовых вопросов владельцу нет обоснованно. Задача может переходить
|
||||
в «Готово к разработке».
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/579-webview-pinch-compositor`, коммит `856a2c6e1238` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `35e12ab83a88ef61b8284ef386341e4e71f40b89`
|
||||
```
|
||||
git log --all --format='%H %T' | grep 35e12ab83a88
|
||||
```
|
||||
- Тело issue: `4545e4010a0a0d13588d41c73912f543b83f5091653de15127390b77630498a0`
|
||||
- Вердикт конвейера: `green` · High 0
|
||||
Reference in New Issue
Block a user