From 6569dced2312f80510f9c30f35c0231cd1f6f15d Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Tue, 22 Sep 2026 23:58:16 +0000 Subject: [PATCH] docs: review document for #609 Issue: #609 User-Visible: no --- docs/reviews/SPEC-REVIEW-609-r1.md | 178 +++++++++++++++++++++++++++++ 1 file changed, 178 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-609-r1.md diff --git a/docs/reviews/SPEC-REVIEW-609-r1.md b/docs/reviews/SPEC-REVIEW-609-r1.md new file mode 100644 index 00000000..01406c4e --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-609-r1.md @@ -0,0 +1,178 @@ +# SPEC-REVIEW-609-r1 + +## Скоуп + +Issue [#609](https://github.com/Matysh/houseplan-card/issues/609) — оболочка +`hp-dialog[form-shell]` (канва `.content`/`.body`, ширина 560 px, максимальная +высота `min(940px, 100vh − 48px)`, единственный скроллер, fullscreen ≤480 px), +принятая референсом #600, реализована только в нативной ветке `` +(`src/hp-dialog.ts:194-212`). Ветка настоящего `ha-dialog`, которую видит любой +реальный пользователь Home Assistant (`_usesHaDialog()` включается всегда, +когда кастомный элемент `ha-dialog` зарегистрирован — то есть внутри HA +всегда), рендерит form-shell без этой оболочки: `width=medium` (580 px), +никакой канвы, штатный HA-fullscreen только при ширине ≤450 px или высоте +≤500 px. ТЗ живёт в теле issue под `## ТЗ` (решение владельца 2026-09-10, +#517), трек — полный (аналитик назвал нарушенный критерий `small`: риск >3, +затронуты desktop и mobile/touch, контракт проверяется на внешней оболочке +настоящего HA). Это первый заход ревью ТЗ (r1); возврата на правки нет, +раздел «Унаследовано» неприменим. + +Разбирается вся текстовая часть тела issue: предварительные «Что не так» / +«Правка» / `## AC`, затем `## ТЗ` целиком — сценарий, что человек увидит, +проблема, скоуп/не-скоуп, контракт поведения (8 пунктов), UX и доступность, +модель данных и миграция, i18n, критерии приёмки AC1–AC7 (каждый с +доказательством), план автотестов, риски, откат, release-артефакты, блок +«принято предположительно». + +## Как проверялось + +Ревью ТЗ, не код: продуктового кода к этой задаче ещё нет. Сверка каждого +фактического утверждения ТЗ с деревом на SHA `84cffb3f92bba469a987584b262ef6d9efee0d44` +(рабочая копия), чтение канонических документов и трассировка терминологии. + +| Проверка | Результат | +|---|---| +| `docs/SCOPE.md` — попадание в Core user jobs | Администратор (primary persona), правит #600-диалоги настроек в реальном HA — J4/J6 (`zero-to-plan`/`keep the plan true`, два встроенных редактора). Задача чинит регресс уже принятого продуктового решения (#600), а не добавляет новое — конфликтов с lock-инвариантом и «never delete a user's file» нет (геометрия/канва диалога, не данные и не actuation) | +| `src/hp-dialog.ts:197-211` (нативная form-shell оболочка) | Совпадает: `:host([form-shell])` задаёт `--hp-dialog-wide-width: 560px` (198), `max-height: min(940px, calc(100vh - 48px))` (199), канву `--secondary-background-color` (200-206), `@media (max-width: 480px)` fullscreen (209-212) | +| `dialogs.styles.ts:577-582` (`.body` без собственного скроллера/padding) | Совпадает буква в букву: `hp-dialog[form-shell] .body { max-height: none; overflow: visible; padding: 16px; ... }`, мобильный `padding: 12px` на 584 | +| `src/hp-dialog.ts` — ветка `ha-dialog` (:558/574 в тексте issue) | **Устарело на ~8 строк**: `width=medium/small` сейчас на строках 566/582, а не 558/574 — см. «Находки», Low. По существу утверждение верно: ни один form-shell-специфичный стиль/CSS-переменная в эту ветку не идёт, `.content`/`.surface`-правил нет вообще | +| Пять потребителей `form-shell` (space, settings, room, marker, onboarding) | `src/editors/space-settings-dialog.ts:33` (`data-kind="space" form-shell wide`), `general-settings-dialog.ts:82` (`data-kind="settings"`), `room-settings-dialog.ts:146` (`data-kind="room"`), `marker-dialog.ts:848` (`data-kind="marker"`), `houseplan-onboarding-runtime.ts:627` (`data-kind="onboarding"`) — все пять существуют, названия `data-kind` в АC3 ТЗ («Space, General, Room, Device и onboarding») — синонимы этих же пяти значений, не новая номенклатура | +| Реальный HA-fixture для AC1/AC2/AC6/AC7 | `demo/helpers/ha-dialog-fixture.mjs` + `ha-dialog-assets.mjs` существуют, `HA_DIALOG_PIN.version === '20260729.7'` — совпадает с версией, названной в АС1 ТЗ. Офлайн (WebSocket выбрасывает `Error`, `hassConnection` никогда не резолвится) — соответствует АС7 «без внешних запросов/WebSocket». Прецедент использования — `capture_summary_panel_505.mjs` (#505) и `demo/verify_ha_dialog_discard_recovery.mjs` (#607, kind: marker/room/space/settings) | +| `--ha-dialog-width-md` — заявлено в §15 как «принято предположительно» | Не голая догадка: переменная уже используется в продукте (`src/summary-panel-editor-style.ts:17`, `test/fixtures/summary-panel-editor.css:4`) и разбиралась в `CODE-REVIEW-505-r1/r2.md` как реальный, наследуемый publicly CSS custom property настоящего `ha-dialog`. Корректно помечено как техническое решение автора, не продуктовый вопрос | +| Золотой набор — «нативная ветка без изменений» (AC4) | `demo/golden/harness.mjs`, `demo/golden/run.mjs`, `demo/srv/demo.html` нигде не регистрируют кастомный элемент `ha-dialog` — golden всегда идёт через нативный ``-fallback. Стаб `ha-dialog` регистрируют только отдельные smoke-скрипты (`smoke_dialog_help_clipping.mjs`, `smoke_dialog_modal_recovery.mjs`, `smoke_free_walls.mjs`, `smoke_danger_confirm_branches.mjs`, `smoke_summary_dialog_scroll.mjs`), golden они не касаются. Значит риск того, что CSS-правки для `ha-dialog`-ветки случайно сдвинут пиксели 13 golden-сцен, реально нулевой — заявление AC4 технически обосновано, а не на честном слове | +| «13 golden-сцен диалогов #600» (AC4) | Подтверждено: `docs/reviews/CODE-REVIEW-600-r2.md` — «11 из ТЗ + 2 `room-temperature-dialog-*`» = 13, принято `ed2af1b1`. Число не выдумано для этой задачи, унаследовано из закрытой | +| `docs/USER-GUIDE.ru.md:389-394` | Уже описывает целевое поведение как факт: «Четыре диалога настроек... собраны одинаково. Это одна форма шириной 560 px с одной полосой прокрутки». Это ровно то, что #609 обязано сделать истинным и в реальном HA — задача не меняет документированный контракт, а устраняет расхождение между ним и продакшеном; правка USER-GUIDE не нужна, и ТЗ её обоснованно не требует | +| `docs/TOUCH-SUPPORT.md` — editors best-effort на touch | §7 ТЗ требует не уменьшать существующие 44×44 px и не создавать horizontal overflow — это non-regression, а не новое обещание touch-паритета; согласуется с «editors are desktop-first, best effort on touch» | +| §7.1 обязательные разделы | Все присутствуют под `## ТЗ`: сценарий, что человек увидит до/после, проблема, скоуп, не-скоуп, контракт поведения, UX и доступность, модель данных и миграция, i18n, критерии приёмки AC1–AC7 (у каждого — «Доказательство: …»), план автотестов, риски, откат, release-артефакты, плюс отдельный блок «Принято предположительно, можно менять свободно» | +| Трек и критерий §5 | Аналитик прямо назвал нарушенный критерий (риск >3, desktop+mobile/touch, контракт на внешней оболочке HA) — не «обычный трек» без обоснования | +| Продуктовые вопросы владельцу | Отсутствуют, и обоснованно: автор комментария подтвердил («onboarding/create уже входят в контракт #600, штатный fullscreen сохраняется»). Все технические развилки (`--ha-dialog-width-md` vs альтернативный публичный механизм, где именно задавать канву, имена diagnostic/smoke-файлов) вынесены в §15 как решения автора, не продуктовые | +| Защитные AC и будущее код-ревью | АС6 сформулирован в терминах §2.7 («мутация делает свидетеля красным»), план автотестов п.3 заранее требует регистрации мутантов на потерю ширины/канвы/fullscreen — закрывает требование «AC · чем доказан · чем краснеет» до того, как код-ревью его спросит | + +## Находки + +### Low — строки `:558/574` для ветки `ha-dialog` в преамбуле устарели на ~8 строк + +Раздел «Что не так» (часть тела issue до `## ТЗ`, тоже входит в sha256-якорь) +пишет: `Ветка ha-dialog (:558/574) использует width=medium (580 px)...`. На +проверяемом SHA `84cffb3f` атрибут `width=${this.wide ? 'medium' : 'small'}` +находится на строках **566** и **582**, а не 558/574. Причина — коммит +`9bbf6fb2` («fix(dialog): restore HA modal after rejected close», #607), +слитый в `dev` в тот же день, добавил в файл 8 строк (импорт `live`, +комментарии к `_requestClose`/`rejectClose`) выше проверяемого блока; аудит +#609, судя по всему, писался на снимке до этого слияния. + +По существу утверждение не пострадало: обе ``-ветки (describedBy и +без него) по-прежнему не содержат ни одного form-shell-специфичного правила — +только `width=`, `flexcontent`, `preventScrimClose` и aria-биндинги. Раздел +`## ТЗ` (контракт поведения, AC1–AC7, план автотестов) номера строк вообще не +использует — цитата живёт только в преамбуле и не является частью +исполняемого контракта. + +**Снимается решением ревьюера с записью**, цикл не тратит: устаревшая +строчная ссылка в диагностическом вступлении не меняет ни одного AC и не +вводит разработчика в заблуждение — реализация будет читать живой код, а не +цитату из issue. Автору стоит (не обязательно, при следующей правке тела +issue) поправить `:558/574` → `:566/582`, чтобы преамбула не расходилась с +дальнейшими код-ревью того же issue. + +### Low — план автотестов не называет явно «влияние на производительность: нет» + +DoR (`PROCESS.md` §2.5) требует, чтобы влияние на производительность и +бюджеты было названо явно, включая явное «нет». Раздел «Риски» перечисляет +пять рисков (CSS custom properties HA, padding, overflow/скроллер, mobile +overrides не должны задеть generic-диалоги, забытый onboarding), но нигде в +ТЗ нет отдельной строки о performance/bundle-budget — притом что правка чисто +CSS-декларативная (новые селекторы и custom properties, ни одного нового +вычисления в рантайме), риск объективно нулевой. + +**Снимается решением ревьюера с записью**: правка ограничена статическими +CSS-правилами на уже существующих селекторах, `npm run bundle:budget` +(входит в `gate:small`) и так поймает любой неожиданный рост бандла +автоматически. Отсутствие явной строки не создаёт риска, который не был бы +уже закрыт существующим гейтом — но пункт DoR формально не закрыт текстом, и +автору стоит добавить одну фразу («Performance: нет — только CSS») при +следующей правке тела issue, если такая случится по другой причине. + +## Что проверено и корректно + +- Оба продуктовых вопроса §7.1 (какая персона/поверхность/момент и что человек + увидит до/после) отвечены одной фразой без терминов реализации; персона и + задача из `docs/SCOPE.md` названы явно. +- Контракт поведения (8 пунктов) закрывает все обсуждаемые в «Рисках» кейсы: + сжатие ширины на insufficient viewport, безопасная высота с учётом + safe-area, источник канвы и её токен, отсутствие двойного padding, + единственный владелец скролла в обеих ветках, точную границу ≤480 px и её + сосуществование со штатной HA-политикой `height ≤ 500px`, ограничение + правки атрибутом `form-shell` (явный негативный контроль generic-диалогов), + токены темы без хардкода цвета. +- AC1–AC7 — у каждого назван способ доказательства (адресная диагностика на + закреплённой офлайн-фикстуре #505, дешёвый HA-подобный smoke с таблицей + пяти `data-kind` и отрицательным generic-контролем, golden 13 сцен, + фокусные smokes четырёх форм, мутанты на потерю ширины/канвы/fullscreen, + изоляционные проверки самой фикстуры). AC6 заранее сформулирован в + терминах будущего код-ревью («чем краснеет»), что закрывает требование + §2.7 до того, как код-ревью его спросит. +- Скоуп/не-скоуп разделены по существу: содержимое форм, backend, миграции, + generic `hp-dialog`, приватный shadow DOM настоящего HA, штатная широкая- + но-низкая HA-fullscreen-политика — явно вне скоупа, что отсекает + расползание задачи на #600 повторно. +- Технические решения (какой именно публичный механизм CSS-переменных + использовать, где физически задавать канву, имена diagnostic/smoke-файлов) + вынесены в явный блок §15 «принято предположительно» — ровно как требует + §7.1; продуктовых вопросов владельцу нет, и это обоснованно: все развилки + видимого поведения уже зафиксированы принятым #600 и текущим кодом. +- i18n («новых строк нет»), модель данных/миграция («не нужна, изменение + представительное и откатываемое»), откат (revert коммита + пересборка + bundle-копий) закрыты явно, не пропущены молчанием. +- Golden-риск для AC4 проверен по инфраструктуре, а не на честном слове: + demo/golden харнес нигде не регистрирует `ha-dialog`, поэтому изменения + CSS для этой ветки физически не могут задеть 13 принятых сцен. +- `docs/USER-GUIDE.ru.md` уже описывает целевое поведение (560 px, один + скроллер) как факт — задача синхронизирует продакшен с уже документированным + контрактом, а не меняет его; корректно, что release-артефакты не требуют + правки USER-GUIDE. + +## Чего не проверял + +- Не проверял исполнением ни один тест/smoke/diagnostic — на этапе ревью ТЗ + кода задачи ещё нет, оценивался только текст спецификации против дерева на + `84cffb3f`. +- Не проверял актуальность закреплённой версии `home-assistant-frontend==20260729.7` + против последней реальной версии HA — вне скоупа и ТЗ, и этого ревью + (обновление pin не входит в «Скоуп» задачи). +- Не проверял `docs/ARCHITECTURE.md`/`docs/UX-MODES.md` построчно на предмет + описания модального контракта form-shell — целевой grep по `form-shell` + совпадений вне уже проверенных мест не дал; риска расхождения с + архитектурной документацией не нашёл, но не исключаю, что она просто не + описывает этот уровень детализации CSS. +- Не оценивал итоговую визуальную корректность (действительно ли 560 px + + канва будут выглядеть идентично `pairs/room-light.png` в реальном HA) — + это предмет исполнения и последующего код-ревью с самой диагностикой, а не + текста ТЗ. + +## Вывод + +ТЗ полно по §7.1, каждый AC однозначен и снабжён способом доказательства, +технические предположения явно выделены в §15 и не требуют решения +владельца, продуктовые вопросы отсутствуют обоснованно. Фактические +утверждения о коде проверены построчно на текущем SHA и совпадают, за +единственным исключением — устаревшая ссылка на номера строк в преамбуле +(Low, не в `## ТЗ`, не входит в исполняемый контракт). Риск для golden-набора +(AC4) проверен по инфраструктуре и реально нулевой. Обе находки — Low, +снимаются ревьюером с запиской, цикл не тратят. + +**Вердикт: зелёный.** + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `84cffb3f92bb` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `3e2a1d462abbbb8e06d11d07851c6e928e814330` + ``` + git log --all --format='%H %T' | grep 3e2a1d462abb + ``` +- Тело issue: `b08b9257ed688a47f3a0f422cdddc1bfc7cf7c511e81ada3ceeac8c2ce354d51` +- Вердикт конвейера: `green` · High 0