Files
houseplan-card/docs/reviews/SPEC-REVIEW-508-r1.md
2026-09-09 19:01:45 +00:00

18 KiB
Raw Permalink Blame History

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) ни колесом, ни тачем, хотя на демо-стенде (нативная ветка <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 и нативная ветка <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