diff --git a/docs/reviews/SPEC-REVIEW-607-r1.md b/docs/reviews/SPEC-REVIEW-607-r1.md new file mode 100644 index 00000000..60c3eab2 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-607-r1.md @@ -0,0 +1,148 @@ +# SPEC-REVIEW-607-r1 + +## Скоуп + +Issue [#607](https://github.com/Matysh/houseplan-card/issues/607) — в реальном +Home Assistant (ветка `ha-dialog`, не нативный `` демо-стенда) крестик +диалога настроек + «Продолжить» в подтверждении «Отменить изменения?» оставляет +диалог невидимым с живым состоянием формы до перезагрузки страницы. ТЗ живёт в +теле issue под заголовком `## ТЗ` (решение владельца 2026-09-10, #517), трек — +полный (аналитик назвал нарушенный критерий §5: не проходит «одна поверхность», +несёт host/input-риск). Это первый заход ревью ТЗ (r1), возврата на правки нет, +раздел «Унаследовано» не применим. + +Разбирается вся ТЗ-часть тела issue: сценарий, скоуп/не-скоуп, контракт +поведения, UX, модель данных, i18n, AC1–AC4, план автотестов, риски, откат, +release-артефакты, блок технических предположений — плюс предшествующая ей +часть тела (симптом/механизм/предварительный `## AC`), поскольку sha256-якорь +ревью считается по нормализованному телу issue целиком +(`scripts/review-doc-guard.mjs:275-277`), а не только по секции `## ТЗ`. + +## Как проверялось + +Не ручное тестирование продукта (для этапа ТЗ его и не может быть) — сверка +каждого фактического утверждения ТЗ с текущим кодом на SHA +`61d51dd934d5cfc7b7286be51b462d71844da88e`, чтение канонических документов и +трассировка терминологии. + +| Проверка | Результат | +|---|---| +| `docs/SCOPE.md` — попадание в Core user jobs | J4/J6 (администратор безопасно настраивает план без потери черновика) — подтверждено, конфликтов с «never delete a user's file» и lock-инвариантом нет (задача не трогает locks/actuation) | +| Механизм бага: `src/hp-dialog.ts` (`_requestClose`, `rejectClose`, оба `@closed=${this._requestClose}` в ha-dialog-ветке) | Строки 449–462, 564, 579 — совпадают с заявленными в issue. `rejectClose()` действительно только сбрасывает `_closing` и вызывает `requestUpdate()`; биндинг `.open=${true}` неизменен между рендерами, Lit его не диффует — заявление не догадка, а точное чтение кода | +| Четыре хоста используют `rejectClose()` на отказе от discard | `src/editors/marker-dialog.ts:171,180`, `room-settings-dialog.ts:109,118`, `general-settings-dialog.ts:43,51`, `space-form.ts:113,122` — паттерн `if (discard) close(); else dialog?.rejectClose?.();` подтверждён во всех четырёх (строки в ТЗ отличаются от фактических максимум на 1 — граница блока, не по сути) | +| Ссылки на `CODE-REVIEW-600-r1.md:84` и `-r2.md:132` | Точное совпадение номеров строк и содержания: «живые ha-dialog» / «настоящий ha-dialog» не поднимались ни в одном из раундов #600 | +| `demo/helpers/ha-dialog-fixture.mjs` + `capture_summary_panel_505.mjs` | Файл существует, `grep` по репозиторию подтверждает единственного текущего потребителя — `capture_summary_panel_505.mjs` (плюс упоминание в `scripts/check-inputs.mjs:63`), как и написано в ТЗ. `launchHaDialogFixture` — офлайн, без HA-сервера/auth/WebSocket, с pin-проверкой оригинального `home-assistant-frontend==20260729.7`: план автотестов «переиспользовать безопасную loopback-фикстуру #505» реалистичен | +| Четыре существующих `smoke_*_settings_form` (AC3) | `demo/smoke_device_settings_form.mjs`, `smoke_room_settings_form.mjs`, `smoke_space_settings_form.mjs`, `smoke_general_settings_form.mjs` — все существуют | +| `test/hp-dialog-contract.test.mjs` (упомянут как возможное место для нового теста) | Существует, сейчас покрывает только #508 (`flex-content`); ТЗ формулирует его как «либо новый соседний contract test» — не жёсткая привязка, ложных ожиданий не создаёт | +| Терминология кнопок подтверждения: «Отменить изменения?» / «Отменить» (discard) / «Продолжить» (keep-open) | `src/i18n/settings/ru.json:46-49` — `dialog.discard_title`="Отменить изменения?", `discard_confirm`="Отменить", `discard_keep`="Продолжить». Сценарий и AC1–AC3 внутри `## ТЗ` используют «Продолжить» корректно и согласованно с кодом и `docs/USER-GUIDE.ru.md:409-411` | +| `docs/ARCHITECTURE.md:189-197` («One modal contract») | Нативная ``-ветка уже содержит аналогичный self-heal паттерн («если retained open flag пережил top-layer entry — закрывает и переоткрывает нативный диалог один раз»). Предлагаемый в ТЗ подход (двухфазная перестановка `open` либо пересоздание HA-shell) архитектурно согласован с уже принятым в проекте решением для другой ветки, не изобретение с нуля | +| §7.1 обязательные разделы ТЗ | Все присутствуют под `## ТЗ`: сценарий, что человек увидит до/после, проблема, скоуп, не входит, контракт поведения, UX, модель данных и совместимость, i18n, критерии приёмки AC1–AC4 (каждый с «Доказательство: …»), план автотестов, затронутые файлы, производительность и размер, touch и доступность, риски, откат, release-артефакты, плюс отдельный обязательный блок «Принятые технические предположения» | +| Трек и критерий §5 | Аналитик назвал нарушенный критерий («не проходит критерий одной поверхности и несёт host/input-риск») — соответствует требованию §2.2/§5: не «обычный трек» без обоснования | +| Продуктовые вопросы владельцу | Отсутствуют — и обоснованно: ожидаемый результат, варианты Continue/Discard и обязательность сохранения нативной ветки уже однозначно зафиксированы существующим кодом/i18n/UX, гадать не о чем. Все технические решения (где чинить — в `HpDialog.rejectClose()`; как переоткрывать — two-phase `open` либо пересоздание) вынесены в блок предположений «можно менять свободно», что и требует §7.1 | + +## Находки + +### Low — предварительный блок `## AC` в начале тела issue называет несуществующую кнопку + +Перед секцией `## ТЗ` в теле issue (часть исходного аудита, тоже входит в +sha256-якорь) стоит: + +``` +## AC +- AC1. ... крестик HA → «Продолжить» → диалог виден, черновик тот же. ... +- AC2. «Не сохранять» закрывает и обнуляет состояние; чистый диалог закрывается без вопроса. +- AC3. Нативная ветка не меняет поведения ... +``` + +Кнопки с текстом «Не сохранять» в продукте нет: подтверждение «Отменить +изменения?» имеет ровно две кнопки — `discard_confirm`="Отменить" (закрывает и +сбрасывает черновик) и `discard_keep`="Продолжить" (возвращает форму, это и +есть баг). Сама секция `## ТЗ` ниже эту ошибку не повторяет: её +«### Критерии приёмки» (AC1–AC4) и «### Контракт поведения»/«### UX» везде +корректно используют «Отменить»/Discard. Формальный, действующий список AC — +именно в `## ТЗ`, поэтому находка не блокирует разработку: реализация и +код-ревью будут сверяться с корректной секцией. Риск чисто читательский — +два непронумерованных одинаково списка `AC1/AC2/AC3` в одном теле issue, один +из которых называет кнопку, которой нет. + +**Снимается решением ревьюера с записью**, не требует возврата на цикл: +устаревший предварительный список из аудита семантически поглощён разделом +`### Критерии приёмки` внутри `## ТЗ`, который и является контрактом для +разработки и код-ревью. Автору стоит (не обязательно, до следующей правки +тела issue) убрать или явно пометить как «дублируется, см. ТЗ» верхний блок +`## AC`, чтобы не тратить внимание будущего читателя. + +## Что проверено и корректно + +- Все фактические утверждения ТЗ о механизме бага и о существующем коде + (номера строк в `src/hp-dialog.ts` и в четырёх хостах, ссылки на + `CODE-REVIEW-600-*`, существование `ha-dialog-fixture.mjs` и его текущего + единственного потребителя, существование четырёх `smoke_*_settings_form`) + сверены с деревом на SHA `61d51dd9` — совпадают буква в букву, включая + номера строк. Автор не выдал догадку за факт: там, где утверждение не + проверено исполнением («проверяется настоящим компонентом, а не только + стабом» в разделе «Риски»), это явно названо риском, а не тихо принято как + решённое. +- Сценарий и «что человек увидит до/после» отвечают на оба продуктовых + вопроса §7.1 одной фразой без терминов реализации, персона и поверхность из + `docs/SCOPE.md` названы (администратор, настройки устройства/комнаты/ + пространства/общих настроек, реальный HA). +- Контракт поведения (7 пунктов) закрывает все обсуждаемые риски: обычное + восстановление, восстановление после того как `ha-dialog` уже обработал + `closed`, сохранность черновика, защита от повторного `hp-close`/цикла, + отсоединённый элемент/подтверждённый отказ, чистое закрытие без вопроса, + сохранение семантики нативного fallback. +- AC1–AC3 — каждый с доказательством (адресный diagnostic/smoke на закреплённой + офлайн-фикстуре HA либо существующие form smokes), AC4 — защитный AC и сразу + сформулирован в терминах будущего код-ревью («способная покраснеть при + удалении guard»), что заранее закрывает требование §2.7 о «чем краснеет». +- Скоуп/не-скоуп разделены чётко и по существу отсекают соседние темы + (тексты/расположение кнопок, логика Save/Discard, замена `ha-dialog` + собственной оболочкой, обновление версии закреплённого HA frontend) — + предотвращает расползание задачи. +- Технические решения (где чинить, как переоткрывать поверхность, формат + адресного теста) вынесены в явный блок предположений «можно менять + свободно» — ровно так, как требует §7.1, продуктовых вопросов владельцу нет + и обоснованно нет. +- Откат, i18n («нет новых строк»), миграция («не нужна»), touch (не создаёт + нового жеста, только распространяет фикс на touch-активацию крестика) — + все явно закрыты, а не пропущены молчанием. +- Архитектурная согласованность подтверждена: `docs/ARCHITECTURE.md` уже + фиксирует аналогичный self-heal паттерн для нативной ветки — предполагаемый + в ТЗ подход не является новым классом решения для проекта. + +## Чего не проверял + +- Не проверял исполнением ни один тест/smoke — на этапе ревью ТЗ кода ещё + нет, оценивался только сам текст спецификации против текущего дерева. +- Не проверял актуальность версии `home-assistant-frontend==20260729.7`, + зафиксированной в `ha-dialog-assets.mjs`, против последней реальной версии + HA — вне скоупа ревью ТЗ и вне скоупа самой задачи («обновление версии + закреплённого HA frontend» прямо исключено из «Не входит»). +- Не проверял `docs/UX-MODES.md` целиком построчно — целевой grep по + `rejectClose`/`hp-close`/`ha-dialog` совпадений не дал; относящийся контракт + диалогов канонически описан в `docs/ARCHITECTURE.md` §5 «One modal + contract», который проверен. + +## Вывод + +ТЗ полно по §7.1, каждый AC однозначен и снабжён способом доказательства, +технические предположения явно выделены и не требуют решения владельца, +фактические утверждения о коде проверены построчно и не являются догадкой. +Единственная находка — Low, снимается ревьюером с запиской, цикл не тратит. + +**Вердикт: зелёный.** + +--- + + + +## Материал раунда + +- Ветка: `issue/607-ha-dialog-close`, коммит `61d51dd934d5` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `11896314f247ec0cb06bd20fbe0205751b9f7652` + ``` + git log --all --format='%H %T' | grep 11896314f247 + ``` +- Тело issue: `c78f51ffc0d5268ea3a82d980f9061b9f2756d0a850072fec514342889af8c9e` +- Вердикт конвейера: `green` · High 0