diff --git a/docs/reviews/SPEC-REVIEW-613-r1.md b/docs/reviews/SPEC-REVIEW-613-r1.md new file mode 100644 index 00000000..fd51f1f9 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-613-r1.md @@ -0,0 +1,231 @@ +# SPEC-REVIEW-613-r1 + +Материал: тело issue #613 (первая редакция, комментарий-аналитика от Matysh +2026-09-23T02:02:47Z уже приложен, ТЗ после него не менялся). Дерево кода на +момент проверки — `1241b9499c7b33d4a4b905d884f60ae6db74690c` (`dev`), читано +только для проверки утверждений ТЗ о существующем коде, не как материал +код-ревью. + +Заход r1, блокирующих циклов израсходовано 0 из 4 (ещё не потрачены — это +первый заход, полный трек). + +## Скоуп ревью + +Issue #613: дефект #1 (устаревание hit-индекса устройств после прокрутки +дашборда/страницы) и дефект #2 (`_saveZoom()` синхронно пишет `localStorage` +на каждый `pointermove` обоих pinch-путей). ТЗ полного трека, тело issue, +раздел `## ТЗ`. Проверялись: наличие обязательных разделов §7.1, однозначность +и доказуемость AC1…AC6, соответствие описания текущему коду +(`src/device-hit-owner.ts`, `src/houseplan-card.ts`), соответствие +`docs/SCOPE.md` (J1/J3) и `docs/TOUCH-SUPPORT.md` (View/kiosk — +release-blocking), а также техническая реализуемость заявленного подхода к +scroll-инвалидации. + +## Как проверялось + +- Прочитан `docs/SCOPE.md`, релевантные разделы `PROCESS.md` (§1–§9, лимит + циклов §4, лёгкий трек §5, шаблоны §7.2) и `docs/TOUCH-SUPPORT.md` целиком. +- Прочитано тело issue #613 и единственный комментарий (аналитика). +- Прочитан `src/device-hit-owner.ts` целиком (288 строк) — подтверждён + механизм `DeviceHitIndex`/`DeviceHitController`/`DevicePointerOwnerLatch`, + на который ссылается ТЗ (арбитраж #564). +- В `src/houseplan-card.ts` найдены и прочитаны оба pinch-пути: обычный stage + (`_stagePointerMove` / `_stagePointerUp` / `_stagePointerCancel`, строки + ~6903–7113, 7964–7981) и capture-guard для жеста, начатого над устройством + (`_guardTouchGesture`, строки ~7329–7413) — подтверждено, что `_saveZoom()` + (строка 6968 и 7388) вызывается на каждый `pointermove` в обоих путях и что + ни один терминальный обработчик (`pointerup`/`pointercancel`/ + `lostpointercapture`) сейчас не делает единственную финальную запись. +- Поиском по `src/houseplan-card.ts` подтверждено отсутствие любого текущего + `scroll`/`visualViewport` слушателя — инвалидация индекса сейчас идёт только + через `ResizeObserver(.stage)` и явные вызовы `_invalidateDeviceHitGeometry()` + при изменении геометрии устройств. Заявление ТЗ «сейчас он инвалидируется при + ResizeObserver... но не при прокрутке» подтверждено кодом, не домыслено. +- Подтверждено существование `_deviceHitOwnerAt` (строка 5718) и обоих + smoke-файлов, которые ТЗ предлагает расширить: `demo/smoke_device_hit_capsules.mjs`, + `demo/smoke_editor_gestures.mjs` — план автотестов ссылается на реальные + файлы, а не на выдуманные имена. +- Найдено на будущее свойство: `connectedCallback`/`disconnectedCallback` в + `src/houseplan-card.ts` (2672, 2780) существуют и уже несут прецедент + «таймеры/подписки умирают на disconnect» (комментарий AUD-1552-01) — AC2 + (lifecycle слушателей) реализуем на существующем паттерне. +- **Ключевая проверка — техническая реализуемость принятого предположения + «capture-listener `scroll` на `ownerDocument`».** Эмпирически проверено в + реальном Chromium проекта (`playwright`, тот же движок, что и в + `demo/serve.mjs`): скролл элемента внутри `shadowRoot` **не долетает** до + `document`-уровневого слушателя даже с `{capture: true}` — см. находку H1 + ниже, вывод теста приведён там же дословно. +- Проверено по коду: в проекте уже есть прецедент подъёма по цепочке теневых + хостов через `getRootNode()` (`src/hp-dialog.ts:329,464-465`, + `src/hp-zigbee-topology-overlay.ts:80,165`, + `src/summary-panel-runtime-loaded.ts:396`) — то есть команда уже решала + именно эту категорию проблемы в других местах кода, просто не в этом ТЗ. +- Автотесты/гейты в этом раунде не гонялись: на стадии `spec` не существует + ни кода, ни коммитов для проверки — гонять `tsc`/`test`/`build` не на чем. + Единственная фактическая проверка — приведённый выше playwright-эксперимент, + подтверждающий конкретное утверждение о поведении браузера. + +## Находки + +### H1 — High. Принятое предположение о scroll-инвалидации не работает в основном заявленном сценарии (HA-дашборд), и это не покрыто ни одним AC + +**Файл:** тело issue #613, разделы «Принятые технические предположения» и +«Риски». + +**Сценарий отказа:** Раздел «Принятые технические предположения» выбирает: +«Использовать capture-listener `scroll` на `ownerDocument` плюс +`visualViewport.scroll/resize`, а не подписываться на динамический список +предков». Раздел «Риски» тут же, тремя строками выше, называет ровно +противоположный факт: «Прокрутка может происходить внутри shadow DOM/ +HA-контейнера, поэтому слушатель только на `window` не покрывает все случаи» — +но противоречие не разрешается: assumption выбирает именно ту схему, которую +risk описывает как недостаточную, и не говорит, почему это приемлемо или как +будет проверено. + +Это не гипотетическое опасение. Проверено экспериментально в реальном +Chromium (том же движке, что использует `demo/serve.mjs`/`playwright`): + +```js +const host = document.getElementById('host'); +const root = host.attachShadow({ mode: 'open' }); +root.innerHTML = '
' + + '
'; +document.addEventListener('scroll', () => window.__docScrollSeen++, true); +// прокрутка #scroller внутри теневого корня: +root.getElementById('scroller').scrollTop = 50; +// → window.__docScrollSeen остаётся 0 +``` + +Результат: `{ doc: 0, win: undefined }` — `document`/`window` capture-слушатель +**не видит** scroll, произошедший на элементе внутри отдельного +`shadowRoot`, даже с `capture: true`. Причина: событие `scroll` не +`composed`, поэтому его путь диспетчеризации не покидает теневое дерево, в +котором оно возникло, — не помогает ни capture, ни bubble. + +Сценарий issue («карточка занимает часть прокручиваемого дашборда Home +Assistant») — это именно многоуровневая Shadow DOM структура +(`home-assistant` → `home-assistant-main` → `ha-panel-lovelace` → `hui-view` и +т.д., каждый уровень — свой `shadowRoot`), и сама кодовая база это признаёт +косвенно (в ней уже есть прецеденты подъёма по `getRootNode()` именно для +обхода этой границы: `src/hp-dialog.ts:329,464-465`, +`src/hp-zigbee-topology-overlay.ts:80,165`). Если реальный скроллящийся +предок дашборда находится в чужом (не `document`) теневом дереве — что для HA +Lovelace типично, — выбранный слушатель на `ownerDocument` его не увидит, и +дефект №1 («после прокрутки тап переключает соседнее устройство») в реальном +эксплуатационном сценарии останется неисправленным. + +**Почему это блокирует, а не «инженерная деталь на усмотрение реализации».** +AC1 и AC2 доказываются smoke-тестом на «прокручиваемой странице/контейнере» +без указания, что этот контейнер обязан быть за пределами `document`-дерева +самой карточки (внутри отдельного `shadowRoot`, как в реальном HA). Смок, +построенный на простом `
` в light DOM, зазеленеет +и «докажет» AC1/AC2 — но не докажет исправление сценария, ради которого issue +заведён: пользователь листает Lovelace-дашборд HA, где скролл-контейнер +почти наверняка находится в стороннем теневом дереве. Это ровно тот случай, +когда «AC формально выполнены» и «сценарий не решён» расходятся (жёлтый +вердикт допустим и при выполненных AC). + +**Что нужно от автора при возврате:** либо (а) заменить/дополнить механизм +инвалидации на способ, не зависящий от `composed`-пути `scroll` — например, +подъём по реальной цепочке скроллящихся предков через `getRootNode()` +(прецедент уже есть в проекте) или наблюдение за геометрией независимо от +scroll-событий (`IntersectionObserver`, который штатно пересекает границы +теневых деревьев), — либо (б) явно ограничить AC1/AC2 договорённой +доказуемой моделью и вынести недостающее покрытие в названный, а не молчаливый +риск с решением, что с ним делать. Раздел «Риски» такой решённости сейчас не +даёт — он просто называет факт и переходит к следующему пункту. + +### M1 — Medium (в скоупе), сопутствует H1. AC1/AC2 не требуют, чтобы smoke-контейнер пересекал границу shadow DOM + +**Файл:** тело issue #613, раздел «Критерии приёмки», AC1 и AC2; раздел «План +автотестов». + +Даже если H1 будет закрыт правильным техническим решением, формулировка +AC1 («два реальных маркера... в прокручиваемой странице/контейнере») и AC2 +не требуют, чтобы хотя бы один тестовый сценарий воспроизводил скролл именно +**внутри стороннего `shadowRoot`**, имитирующего реальное вложение карточки в +Home Assistant. Без этого требования разработчик может закрыть AC на light-DOM +контейнере, интуитивно решить, что «прокрутка есть — тест прошёл», и не +заметить, что боевой сценарий не покрыт. Это отдельная от H1 находка, потому +что даже правильная реализация нуждается в тесте, который умеет упасть именно +на этом классе регрессии — иначе AC6 (мутационная защита) будет мутировать +код, который сам тест не отличает от рабочего в реалистичном вложении. + +Правится вместе с H1 в этом же цикле: формулировка AC1/AC2 должна явно +называть, что хотя бы один прогон использует scroll-контейнер за пределами +`ownerDocument` дерева карточки (внутри отдельного `attachShadow`), а не +только «прокручиваемую страницу». + +## Что проверено и корректно + +- Все обязательные разделы §7.1 присутствуют: сценарий, что человек увидит до + и после, проблема, скоуп/не-скоуп, контракт поведения (7 пунктов), UX, + модель данных и миграция, i18n, AC1…AC6 с указанием способа доказательства, + план автотестов, риски, откат, release-артефакты. +- Оба технических утверждения о текущем поведении кода (два pinch-пути пишут + `_saveZoom()` на каждый `pointermove`; индекс не инвалидируется на scroll) + подтверждены чтением `src/device-hit-owner.ts` и `src/houseplan-card.ts`, а + не приняты на слово автора. +- Ссылка на арбитраж #564 (латч владельца pointer-последовательности) — + подтверждена структурой `DevicePointerOwnerLatch` в `device-hit-owner.ts`; + контракт «прокрутка не меняет владельца уже начатой последовательности» + реализуем на существующем механизме без структурных изменений. +- AC1–AC6 однозначны и каждый называет способ доказательства (`smoke`, + `unit/contract + smoke`, `mutation`); AC6 корректно требует двух раздельных + мутантов на две независимые защиты (scroll-инвалидация и terminal-only + persistence), что соответствует §2.7 (таблица «чем краснеет»). +- Не-скоуп сформулирован явно и исключает ровно те смежные контракты (#563, + #578, размеры капсул, порядок перекрытий, рендер), которые действительно не + должны меняться этой задачей. +- «Принятые технические предположения» оформлены отдельным блоком с пометкой + «можно поменять свободно» — процессуально корректно; H1 — это ровно + реализация права ревьюера оспорить такое предположение (§7.1), а не + претензия к оформлению. +- Продуктовых вопросов к владельцу нет и не требуется: всё найденное — + техническое, разрешается на этом ревью, не эскалируется. +- Track (полный) обоснован верно: две поверхности/подсистемы (hit-index + + zoom persistence) и прямое влияние на touch-контракт/perf горячего пути — + критерии `small` действительно не выполняются, «лёгкий трек» здесь был бы + неверным выбором. +- Влияние на i18n, миграцию, откат — корректно и полно названы как «нет»/ + «revert коммита», без домыслов. + +## Чего не проверял + +- Не запускал `tsc`/`test`/`build`/смоки — на стадии ТЗ нет кода для прогона; + единственный практический тест в этом раунде — точечный playwright-скрипт, + подтверждающий утверждение о поведении `scroll`-событий относительно + `shadowRoot` (см. H1), не гейт продукта. +- Не проверял реальную DOM-структуру актуальной версии Home Assistant + Lovelace (`hui-view`/`ha-panel-lovelace`) вживую — вывод о «типичной + многоуровневой Shadow DOM» опирается на общеизвестную архитектуру HA + frontend и на то, что сама кодовая база уже решает эту границу в других + местах (`getRootNode()` прецеденты), а не на прямое наблюдение внутри этого + ревью. +- Не оценивал производительность предложенного IntersectionObserver-подобного + fallback (это было бы решением за автора, не моя роль на этом этапе). +- Не проверял issues #563/#578/#582 построчно — они прочитаны только по + ссылкам из аналитики #613 для проверки отсутствия дублирования; сам их код + вне скоупа этой задачи (заявлено ТЗ явно, и это корректно). + +## Вывод + +Один High (H1) и один сопутствующий Medium-в-скоупе (M1), оба технические, +оба возвращаются автору ТЗ в этом же issue. Остальная часть ТЗ методологически +и процессуально готова — переработка ожидается точечной (раздел +«Принятые технические предположения» + формулировка AC1/AC2), не переписыванием +задачи целиком. + +--- + + + +## Материал раунда + +- Ветка: `issue/613-scroll-hit-pinch-persist`, коммит `1241b9499c7b` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `9a4518ab2014ca6190468cd459762dedbcf1ff0f` + ``` + git log --all --format='%H %T' | grep 9a4518ab2014 + ``` +- Тело issue: `40b48efe32b7ca1cbc2bf41d1b3da9d1f55bf042cb41d7fa691c9453fa563e79` +- Вердикт конвейера: `yellow` · High 1