diff --git a/docs/reviews/SPEC-REVIEW-402-r1.md b/docs/reviews/SPEC-REVIEW-402-r1.md new file mode 100644 index 00000000..5ca6506e --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-402-r1.md @@ -0,0 +1,222 @@ +# SPEC-REVIEW-402-r1 + +- Issue: https://github.com/Matysh/houseplan-card/issues/402 +- Артефакт ТЗ: `docs/specs/402-confirm-outside-main-branch.md` (полный трек, класс A) +- Заход: r1 · лимит циклов ревью ТЗ для полного трека — 4 (§4), израсходовано 0 +- Ревьюер: Claude (роль «ревьюер ТЗ»), независимая сессия, без устных пояснений автора +- База сравнения: HEAD `d94db87e` (коммит, добавивший спецификацию) + +## Скоуп ревью + +Первый раунд ревью ТЗ для #402: регресс подтверждения опасного действия +(`hp-confirm`), которое рендерится только в финальной ветке `render()` +(`src/houseplan-card.ts`) и поэтому недоступно/теряется в остальных ветках +(онбординг «нет пространств», `fixed_floor` pending/invalid, `!space`). +Задача полного трека (P1, bug, класс A) — файл ТЗ обязателен, что и сделано. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком (разделы §2.4, + §2.5, §5, §7.1, §7.2), тело issue #402 и единственный комментарий (аналитика + S2). +2. Прочитан весь текст ТЗ `docs/specs/402-confirm-outside-main-branch.md`. +3. Каждое фактическое утверждение ТЗ о коде сверено с текущим деревом на SHA + `d94db87e`: + - структура `render()` (`src/houseplan-card.ts:11165-11884`) — все семь + ранних веток и их точки возврата; + - `HpConfirmController` (`src/danger-confirm.ts`) — поведение `confirm` / + `resolve` / `cancel`; + - вызов `_confirmDanger` из `_deleteServerPlan` + (`src/houseplan-onboarding-runtime.ts:218-224`, клик на `:273`); + - все восемь call site'ов `_confirmDanger` (`houseplan-card.ts`, + `houseplan-editor-runtime.ts` ×5, `houseplan-onboarding-runtime.ts` ×2); + - расположение `_tapConfirm` (`:11852-11868`) и `_vacCalConfirm` + (`:11802-11812`) относительно `_dangerConfirm` (`:11872-11878`); + - импорт `hp-confirm` — проверено, что он не ленивый (`houseplan-card.ts:14`, + верхнеуровневый `import`), что подтверждает заявление ТЗ об отсутствии + влияния на бюджет (AC7); + - использование `noChange` во всём `src/**` — подтверждено, что сентинел + нигде не оборачивается в шаблон (`grep noChange`), это делает техническое + утверждение границы ТЗ («noChange нельзя обернуть») фактом, а не догадкой. +4. Проверено соответствие терминологии `docs/USER-GUIDE.ru.md` (раздел про + общий диалог подтверждения) — видимых текстов ТЗ не меняет, конфликта нет. +5. Проверено `docs/TOUCH-SUPPORT.md` и DoR-чек-лист §2.5 на предмет + обязательного пункта про touch/kiosk — см. находку H1. +6. Проверены обязательные разделы §7.1 — присутствуют все (сценарий · что + человек увидит · проблема/контракт · скоуп/не-скоуп · UX · модель данных · + i18n · AC1-7 с доказательством · план автотестов · риски · откат · + release-артефакты). +7. Гейты кода (`tsc`, `test`, `build`) не гонялись: на этапе ТЗ продуктового + диффа ещё нет, гонять их не над чем (см. «Чего не проверял»). + +## Находки + +### H1 (High, блокирует). ТЗ не называет влияние на touch/kiosk — обязательный, объявленный блокирующим пункт DoR + +**Где**: весь файл `docs/specs/402-confirm-outside-main-branch.md` — ни разу +не упоминает touch, kiosk или `TOUCH-SUPPORT.md` (`grep -i touch|kiosk` по +файлу — ноль совпадений). + +**Почему это находка, а не формальность**. PROCESS.md §2.5 перечисляет пункты +«Готово к разработке» и явно помечает один из них как блокирующий отдельно от +остальных: «влияние на touch по `docs/TOUCH-SUPPORT.md` (**View и киоск — +блокирующие**)», и там же: «Если хоть один пункт не выполнен — статус не +«Готово к разработке»». `TOUCH-SUPPORT.md` со своей стороны требует того же +прямым текстом: «New editor feature specifications … must state one of: +`Touch editor: supported` / `best effort / intentionally degraded` / `not +exposed`». + +Затронутая этим ТЗ поверхность — не абстрактный служебный код: это ровно +диалог `` в состоянии «нет пространств» (онбординг) и в +`fixed_floor` pending/invalid, то есть экран, который согласно +`TOUCH-SUPPORT.md` относится к категории «View dialogs and safe device +actions» — «Fully supported» на touch, без исключений best-effort. Часть +затронутых call site'ов (`houseplan-editor-runtime.ts`, 5 мест) при этом +принадлежит редакторам, для которых допустима best-effort деградация — но +только если она **явно объявлена**, а не подразумевается молчанием. + +**Симптом отсутствия анализа**: без явного утверждения нельзя отличить +«автор проверил — влияния нет» от «автор не думал про touch вовсе». Это +особенно значимо здесь, потому что рефакторинг двигает `hp-confirm` из +внутреннего дочернего узла одной `` в узел, рендерящийся отдельно от +конкретной ветки разметки (см. Риск «двойной рендер диалога» в самом ТЗ) — +у смены места крепления DOM-узла есть техническая возможность задеть +touch-специфичные вещи (portal/DOM-scope для `dialog.showModal()`, +`pointer-events`, `:focus-trap`), даже если по факту не заденет. + +**Фактическая оценка (для экономии цикла)**: содержательно последствий, +скорее всего, нет — `` остаётся тем же кастомным элементом с тем +же shadow DOM и той же логикой модального диалога независимо от того, чьим +прямым потомком в основном дереве он является; ни один AC1-7 не описывает +изменение верстки, жестов или поведения `hp-confirm` самого по себе. Но это +вывод ревьюера, а не факт, зафиксированный автором в ТЗ — а фиксировать его +обязан автор (правило #163 «New editor feature specifications … must state +one of»). + +**Требуемая правка**: добавить в ТЗ явную строку по образцу +`Touch: View/kiosk — fully supported, без изменений (тот же , +маршрут рендера не меняет разметку/жесты диалога)`, и отдельно — +`Touch editor: supported` для пяти call site'ов из `houseplan-editor-runtime.ts`, +если они действительно не деградируют. Это правка одной-двух строк текста, не +кода. + +### M1 (Medium, в скоупе задачи). «Не-скоуп»-обоснование для `_tapConfirm`/`_vacCalConfirm` содержит фактическую неточность + +**Где**: `docs/specs/402-confirm-outside-main-branch.md`, раздел «Скоуп / +не-скоуп»: «Не в скоупе: … `_tapConfirm` и `_vacCalConfirm` — у них своя +механика и свои ветки.» + +**Проверено чтением кода**: `_tapConfirm` рендерится в +`src/houseplan-card.ts:11852-11868`, `_vacCalConfirm` — в `:11802-11812`, +`_dangerConfirm` — в `:11872-11878`. Все три блока лежат в **одной и той же** +финальной ветке `render()` (последний ``, тот же самый шаблон, между +ними нет ни одного `return`). Утверждение «свои ветки» в буквальном +прочтении неверно: у них нет собственных веток `render()` — они делят ровно +ту же ветку, что и `_dangerConfirm` до этого фикса, и подвержены тому же +классу дефекта («открытое подтверждение исчезает при смене ветки», вторая +половина дефекта из аналитики S2), например если многоклиентская +синхронизация (J6, `docs/SCOPE.md`) уводит карточку в `fixed_floor` pending +или обнуляет `model.length` **пока** открыт `_tapConfirm`/`_vacCalConfirm`. + +Отличие от `_dangerConfirm`, которое у ревьюера НЕ вызывает вопросов и, +похоже, и есть настоящая причина исключения: `_tapConfirm`/`_vacCalConfirm` +не используют `HpConfirmController` и не отдают вызывающему `Promise` — +`exec()` вызывается синхронно по клику, поэтому «вечно висящего промиса» у +них в принципе не бывает (при потере диалога они не блокируют await +вызывающего, а просто становятся недоступны до следующего рендера основной +ветки). А сами точки входа (`marker.tap_confirm`, календарь пылесоса) +физически недостижимы из веток онбординга/`fixed_floor`/`!space` — там нет +устройств на плане, значит буквальный сценарий issue их не касается. + +Это делает исключение из скоупа **разумным по существу**, но +**обоснование в тексте — неверным**: «свои ветки» вместо настоящей причины +(«синхронный exec без промиса» + «недостижимость точки входа из веток вне +основной»). Ложное обоснование опасно не абстрактно: тот, кто будет +реализовывать вынос `hp-confirm` из финальной ветки, должен точно знать, что +`_tapConfirm`/`_vacCalConfirm` физически стоят в той же ветке и их нельзя +случайно утащить вместе с `_dangerConfirm` при выносе блока — а формулировка +«у них свои ветки» наводит на обратное представление. + +**Требуемая правка**: заменить формулировку на техническую причину исключения +(синхронный `exec`, недостижимость входа) и явно предупредить реализацию не +трогать расположение этих двух блоков при выносе `_dangerConfirm`. Опционально +(не обязательно для этого issue) — упомянуть остаточный риск «уже открытый +`_tapConfirm`/`_vacCalConfirm` теряется при смене ветки во время +многоклиентской синхронизации» как кандидата в отдельный issue, если владелец +сочтёт его достаточно вероятным; в скоупе #402 фиксировать не обязательно — +issue именно про мёртвую кнопку и висящий промис, которых у этих двух объектов +нет. + +## Что проверено и признано корректным + +- **Диагноз дефекта** — точен и воспроизводим по строкам: `render()` действительно + возвращает `hp-confirm` только в финальной ветке (`:11872`), все более ранние + `return` (`:11166`, `:11170`, `:11171`, `fixed_floor` pending/invalid, + `!model.length`, `!space`) реально существуют и реально не содержат + `hp-confirm`. +- **`HpConfirmController` описан верно**: `cancel()` действительно резолвит + `false` через `resolve()`, `resolve()` сверяет токен, новый `confirm()` + отменяет предыдущий запрос (`src/danger-confirm.ts:47-72`) — контроллер не + участвует в дефекте, претензия ТЗ обоснована. +- **Буквальный сценарий issue** (`_deleteServerPlan` из онбординга) сверен + построчно: `src/houseplan-onboarding-runtime.ts:218-224` вызывает + `this.host._confirmDanger`, кнопка в `:273` — реальная причина «мёртвой + корзины» из issue. +- **Техническое утверждение о `noChange`** («нельзя обернуть в шаблон») — + подтверждено: во всём `src/**` `noChange` возвращается только как значение + `render()` целиком (`houseplan-card.ts`, `editor.ts`, `space-card.ts`, + `space-editor.ts`), ни разу не встречается внутри `${…}` — граница ТЗ не + придумана, а списана с уже действующего паттерна кодовой базы. +- **AC1-AC4, AC6** — проверяемы, у каждого назван способ доказательства + (смок), и способ реалистичен: `demo/smoke_danger_confirmation.mjs` уже + существует и уже покрывает контракт диалога, дополнение под новые ветки — + органичное расширение того же файла. +- **AC5** (иммедиат-отказ в `!_config||!hass` и `warm`) — продуман + корректно: `!hass`/`!_config` не могут стать `false→true→false` в течение + жизни примонтированной карточки (Lovelace не сбрасывает `hass`/`config` в + falsy после первичной установки), то есть эта ветка достижима только до + первого рендера — отказывать там немедленно безопасно и не теряет открытый + диалог (терять нечего). Ветка `warm` реально достижима в течение сессии + (смена языка), и там `noChange` **сохраняет** уже показанный диалог + нетронутым (Lit не трогает DOM) — то есть AC5 верно ограничивает + немедленный отказ только НОВЫМИ запросами, не описывая (и не должен + описывать) отмену уже открытых. +- **AC7 (бюджет)** — `hp-confirm` импортируется в шапке файла обычным + (не ленивым) `import` (`houseplan-card.ts:14`), поэтому перемещение точки + рендера не добавляет новый чанк/зависимость — заявление «правка структурная, + бюджет не растёт» подтверждается фактическим импортом. +- **Скоуп/не-скоуп в остальном** — граница с #32 (содержимое диалога, + ревалидация после `await`) и с #406 (`alertdialog`/`aria-describedby`) + названа явно и не пересекается с контрактом этого ТЗ. +- **Обязательные разделы §7.1** — все присутствуют, включая «откат» и + «release-артефакты» (changelog RU+EN). +- **Соответствие `docs/SCOPE.md`**: чинит J4 (онбординг, «zero to a working + plan») и снимает риск для J6 (multi-client sync может вызвать смену ветки + под открытым диалогом) — задача не расширяет продукт, а восстанавливает + ранее рабочее поведение (регресс против до-#32). +- Метки issue (`bug`, `P1`, `S4-spec-review`, без `small`/`trivial`) + согласуются с заявленным в ТЗ полным треком. + +## Чего не проверял + +- **Гейты кода** (`npx tsc --noEmit`, `npm test`, `npm run build`, + `check-docs`, смоки, `bundle:budget`) — не гонялись: на этапе ТЗ + продуктового кода ещё нет, диффа для гейтов не существует. Это будет + предметом код-ревью после реализации. +- **`scripts/mutation-gate.mjs` / `demo/smoke_danger_confirmation.mjs`** — + не запускал, только убедился, что оба файла существуют и их формат + (реестр мутантов, browser-smoke на реальной карточке) совместим с планом + автотестов ТЗ. +- **Таблицу `docs/specs/README.md`** — строка для #402 в неё не добавлена, + но это не регрессия этого ТЗ: специфика #395-#401 туда тоже не занесена + (таблица не поддерживается систематически, известный долг §7.3 п.1 + документа процесса), поэтому не поднимаю отдельной находкой. +- **Реальный рендер в браузере** — на этапе ТЗ кода нет, воспроизведение + дефекта в описании ТЗ и issue взято на веру как «воспроизведено исполнением» + автором аналитики; независимо не перепроверял (это будет предметом + смок-доказательства в код-ревью). +- **`_tapConfirm`/`_vacCalConfirm` как отдельный дефект** — не завожу + отдельный issue: по результату разбора (см. M1) это не тот же класс + дефекта (нет висящего промиса, нет недостижимости из веток issue), поэтому + это не «Medium вне скоупа» по #202, а неточность формулировки внутри + текущего ТЗ — чинится правкой текста в этом же документе.