diff --git a/docs/reviews/SPEC-REVIEW-508-r1.md b/docs/reviews/SPEC-REVIEW-508-r1.md new file mode 100644 index 00000000..d9340ac7 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-508-r1.md @@ -0,0 +1,195 @@ +# SPEC-REVIEW-508-r1 + +- Issue: #508 «Сводная панель: в диалоге настроек не работает прокрутка — колесом мыши и touch на мобильных» +- Этап: S4-spec-review (PROCESS.md §2.4) +- Трек: `small` (лёгкий) — ТЗ живёт в теле issue, файл в `docs/specs/` не создаётся +- Заход: r1 · блокирующих циклов ревью ТЗ израсходовано 0 из 2 (лимит лёгкого трека, §4) +- Материал: тело issue #508 + комментарий S2 от 09.09 (аналитика + ТЗ, автор Codex) +- Ревьюер: Claude (сессия ревью ТЗ), другая роль/модель, чем автор + +## Скоуп + +Диалог настроек сводной панели (`hp-dialog[data-kind="summary"]`) не прокручивается +внутри реальной HA (`ha-dialog`/`wa-dialog`) ни колесом, ни тачем, хотя на демо-стенде +(нативная ветка ``) работает. ТЗ предлагает точечный фикс: новый opt-in атрибут +`flex-content` на `hp-dialog`, форвардящийся как `?flexcontent` на `ha-dialog` в обеих +HA-ветках рендера; диалог настроек панели получает этот атрибут, остальные — нет. +Продуктовая ценность — J5/J1 (панель обязана быть управляемой с тем же UX, что и +остальные View-поверхности); поверхность одна (`src/hp-dialog.ts`, +`src/summary-panel-editor.ts`), миграции конфига нет, i18n не тронут. Критериям +лёгкого трека (§5) формально соответствует. + +## Как проверялось + +Это ревью ТЗ, кода ещё нет (issue в `S4-spec-review`, S5/S6 впереди) — гейты +`typecheck`/`test`/`build` к этому этапу не относятся, не прогонялись. Проверка — +чтением: `docs/SCOPE.md`, `docs/TOUCH-SUPPORT.md`, `PROCESS.md` §1, §2.4, §2.5, §5, +§7.1; чтением исходников, на которые ссылается ТЗ (`src/hp-dialog.ts` — обе ветки +`ha-dialog` и нативная ветка ``/`.surface`/`.content`; `src/summary-panel-editor.ts`, +`src/summary-panel-editor-style.ts`); проверкой существования файлов и артефактов, +которые ТЗ называет как доказательство (`demo/smoke_summary_panel_polish.mjs`, +`demo/golden/baselines/*`, `demo/capture_summary_panel_505.mjs`, +`demo/helpers/ha-dialog-fixture.mjs`); поиском текста «footer»/«#505» по `docs/*.md`, +поиском `flexContent`/`flexcontent` по репозиторию (не встречается — новая +сущность, конфликта имён нет). + +## Находки + +### Medium (в скоупе задачи — чинится автором в этом же ТЗ) + +**M1. AC3 называет несуществующее доказательство: golden-сценариев `summary-*` в +репозитории нет.** + +- Файл: тело issue #508, раздел «AC», пункт AC3. +- Формулировка: «Нативная ветка (демо) не меняется: `smoke_summary_panel_polish` и + golden `summary-*` без отличий.» +- Проверено: `ls demo/golden/baselines | grep -i summary` → 0 совпадений; ни одного + golden-сценария с префиксом `summary` не существует нигде в `demo/golden/`. + Единственный скрипт, который вообще снимает пиксели диалога настроек панели — + `demo/capture_summary_panel_505.mjs`, и его собственный комментарий в первой + строке файла прямо говорит: «Explicit #505 diagnostic evidence, not a + golden-baseline producer or smoke». То есть покрытия golden-эталонами у этого + диалога нет вовсе — это разовый диагностический скрипт с pinned HA wheel, + осознанно выведенный из обычных гейтов (тяжёлая загрузка ~124 МБ). +- Почему это находка, а не мелочь: команда `npm run golden:verify` (или её часть, + отфильтрованная по несуществующему префиксу `summary-*`) в этом случае сравнит + ноль сценариев и вернёт «нет отличий» **тривиально**, а не потому что поведение + проверено. Это ровно тот антипаттерн, о котором PROCESS.md §2.7 говорит для + код-ревью («тест умеет падать» без свидетеля) — только сдвинутый на этап ТЗ: AC, + который выглядит проверяемым, ссылается на проверку, которая ничего не + проверяет. Код-ревьюер следующего раунда рискует принять «прогнал golden, + summary-* чисто» за доказательство, не заметив, что сравнивать было нечего. +- Что нужно поправить (одно из): (а) убрать golden-часть из AC3 и оставить + доказательством только `smoke_summary_panel_polish` (у него, судя по коду, + и так есть проверки, что нативная ветка ведёт себя ожидаемо); либо (б) явно + снять AC3 до «golden-матрица не даёт отличий» без привязки к + несуществующему префиксу — тогда это то же самое, что говорит AC4, и дублирует + его без потери смысла. Технический выбор — за автором. + +**M2. В ТЗ лёгкого трека нет раздела «откат».** + +- Файл: тело issue #508, весь раздел «## ТЗ (лёгкий трек, после S2 09.09)». +- PROCESS.md §5 задаёт для лёгкого трека фиксированный шаблон тела issue: + «проблема · контракт · AC1…ACn с доказательством · **откат**» — откат назван как + обязательный, наравне с остальными тремя частями, и не отменяется нигде в §5 + для этого случая. ТЗ содержит «Причина / Изменения / Тесты / AC», но ни одной + фразы о том, как вернуть поведение назад, если фикс скажется неожиданно (флага + Labs это не касается — правки штатные, поэтому откат тут дёшев, но он должен + быть **назван**, а не подразумеваться). +- Чем это грозит: это ровно тот пункт DoR (§2.5), который проверяется перед + переводом в `S5-ready» — «откат: как выключить или вернуть назад». Без явной + строки в ТЗ переход в `S5-ready` формально не проходит чек-лист. +- Фикс дешёвый: одна фраза, например «откат — обычный ревёрт коммита; атрибут + `flex-content` opt-in только у диалога настроек панели, у остальных `hp-dialog` + поведение не меняется вообще». + +### Low (на усмотрение автора/ревьюера — не блокирует) + +**L1. Целевой файл документации для фразы про футер не определён.** +Пункт 3 «Изменений» называет цель как «`docs/UX-MODES.md` (или где описан контракт +#505 про футер)». Поиском по `docs/*.md` такого текста про футер сейчас нигде нет +(проверено `grep -rn footer docs/*.md | grep -i summary` → пусто); ближайшее по духу +место — `docs/ARCHITECTURE.md:1983` («The #505 designer-aligned surface…»), где уже +описана техническая сторона диалога настроек. Не блокирует: раздел «Всё, чего +пользователь не наблюдает, агенты решают сами» (`PROCESS.md §7.1`) явно относит +«где лежит документация» к техническим решениям автора, а не к продуктовому +вопросу владельцу. Снимаю с записью: автор выбирает файл сам при реализации. + +**L2. Влияние на производительность не названо явно.** +DoR (§2.5) требует явного «нет», если влияния нет; ТЗ не произносит эту фразу. +Не блокирует: правка — булев CSS-атрибут на существующем диалоге, ничего не +добавляет в горячий путь рендера/анимации панели, перф-риска по чтению кода не +вижу. Снимаю с записью — автору стоит добавить одну строку «перф: нет влияния» +при реализации, чтобы чек-лист DoR не спотыкался формально. + +## Что проверено и корректно + +- **Причинно-следственная цепочка фикса правдоподобна и подтверждена не только + рассуждением.** Автор S2-комментария воспроизвёл баг на реальном стенде + (ha.jbstudio.pro, HA 2026.9.1), измерил `clientHeight`/`scrollHeight` реального + `.body` внутри `ha-dialog`/`wa-dialog» и отдельно подтвердил механизм + (`overflow-y:auto` + `overscroll-behavior:contain` на контейнере без + ограниченной высоты обрывает scroll chaining в Chromium) изолированной + Playwright-страницей. Это не догадка, выданная за факт — гипотеза 1 + (перехват `wheel` обработчиками панели) явно проверена и снята с указанием + файлов и строк (`summary-panel-runtime-loaded.ts:207,236`). +- **Независимое подтверждение в уже существующем коде.** `demo/capture_summary_panel_505.mjs` + (написан раньше, для #505) уже содержит комментарий «The genuine HA owns + scrolling in its shadow `.body`; the native wrapper owns it in the editor. + Follow the composed tree instead of assuming» — то есть проблема с тем, что + `.body` владеет скроллом в реальной HA, была на радаре ещё на #505. Диагноз + ТЗ #508 согласуется с этим, а не противоречит. +- **Структура рендера в `src/hp-dialog.ts` соответствует описанной в ТЗ.** Обе + HA-ветки (`describedBy` есть/нет, строки 504–517 и 519–531) и нативная ветка + (534–557) — ровно то, что называет ТЗ. Нативная ветка действительно уже + ограничена по высоте (`.surface{max-height:92vh;overflow:hidden}` → + `.content{display:flex;flex-direction:column}` → слотированный + `.summary-editor` как единственный скроллер) — заявление «нативной ветке + ничего не нужно» подтверждается чтением, а не на слово. + Атрибут `flexContent`/`flexcontent` нигде в репозитории пока не существует — + конфликта имён с уже используемым нет. +- **Тесты спроектированы с учётом «тест должен уметь падать».** Пункт (в) в + разделе «Тесты» — отдельный сценарий «без `flex-content` у стаба скролл не + идёт» — это ровно негативный свидетель, доказывающий, что стенд + воспроизводит баг, а не только то, что фикс включён. Оба протективных + AC (AC1, AC2) получают названные мутанты + (`summary-dialog-drops-flex-content`, `hp-dialog-ignores-flex-content»), + что заранее закрывает требование PROCESS.md §2.7 про мутант в + `scripts/mutation-gate.mjs» для защиты, проверяемой дорогим (браузерным) + гейтом — это сделано на этапе ТЗ, до того как это стало находкой код-ревью. +- **AC4 и golden-матрица.** В отличие от AC3 (M1), формулировка AC4 «прочие + `hp-dialog` без `flex-content` не меняют раскладку (golden-матрица без + отличий)» опирается на реально существующий и достаточно широкий набор + golden-эталонов с диалогами (`device-dialog-mobile-ru.png`, + `optimize-preflight-dialog-light-ru.png`, `tray-*` и др.) — здесь + доказательство существует и полный прогон `npm run golden:verify` его + даст. Претензия M1 относится только к AC3, не к AC4. +- **Соответствие `docs/SCOPE.md` и `docs/TOUCH-SUPPORT.md`.** Диалог настроек + панели — гарантированная View-поверхность на touch + (`TOUCH-SUPPORT.md:48-49`: «The summary panel, both halves of its control, + and its simple settings form are supported View surfaces»), поэтому touch-часть + AC2 не расширяет скоуп задачи произвольно, а чинит уже гарантированный + контракт — включение touch в задачу правомерно, а не самодеятельность + автора. +- **Скоуп не размыт.** Правка ограничена одним атрибутом на одном диалоге; + остальные `hp-dialog` явно объявлены неприкосновенными (AC4), новой + подсистемы или контракта поведения не появляется. +- **Трек `small` выбран корректно**: одна поверхность, нет миграции конфига, + нет новых i18n-ключей, нет нового UX-контракта (восстанавливается уже + обещанный), touch — часть уже гарантированного, а не нового контракта. + Названного нарушенного критерия для полного трека нет — и это правильно, + такого критерия действительно не видно. +- Открытых продуктовых вопросов к владельцу нет — весь материал технический + и решается автором/ревьюером (что и делает это ревью). + +## Чего не проверял + +- Не запускал никаких гейтов (`typecheck`/`test`/`build`/`golden`/`smoke`) — + на этапе ревью ТЗ кода ещё нет, гейты к этому этапу не относятся. +- Не проверял факт существования атрибута `flexcontent` у реального `ha-dialog`/ + `wa-dialog` независимо от стенда — полагаюсь на воспроизведённое автором + измерение на живом HA 2026.9.1 (S2-комментарий); отдельного подтверждения из + исходников самой HA-фронтенд-сборки в этом репозитории нет и быть не может + (внешняя зависимость, pinned wheel только в диагностическом + `demo/capture_summary_panel_505.mjs`, не в этом дереве). +- Не оценивал качество формулировок будущего юнит-теста + `test/hp-dialog-contract.test.mjs` — файла ещё нет, ТЗ описывает его + проверяемым способом (source-text на оба рендера), к тексту претензий нет. + +## Вердикт + +Находок High нет. Medium — 2, обе в скоупе задачи, обе дёшево чинятся в теле +issue до перевода в `S5-ready` (без нового цикла с моей стороны по существу — +но формально возврат на правки бюджет §4 тратит, раз это не зелёный вердикт). + +Вердикт: жёлтый · заход r1 · блокирующих циклов 1/2 · High: 0 · Medium: 2 → в задаче + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Якоря снять не удалось: ветки задачи нет, материал читался по `dev`. +- Вердикт конвейера: `yellow` · High 0