diff --git a/docs/reviews/SPEC-REVIEW-381-r1.md b/docs/reviews/SPEC-REVIEW-381-r1.md new file mode 100644 index 00000000..1d68f2e2 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-381-r1.md @@ -0,0 +1,223 @@ +# SPEC-REVIEW-381-r1 + +- Issue: https://github.com/Matysh/houseplan-card/issues/381 — «Действие по + нажатию: добавить "Ничего не делать"» +- Этап: ТЗ на ревью (PROCESS.md §2.4), **полный трек** (владелец: нарушен + критерий §5 «нет нового UX-контракта» — новое наблюдаемое действие и новое + сохраняемое значение публичной конфигурации) +- ТЗ: `docs/specs/381-no-op-tap-action.md` +- Материал ревью: ТЗ на ветке `issue/381-no-op-tap-action`, ревизия автора + `7a260e8f`; `git diff --stat origin/dev...HEAD` подтверждает, что изменены + только `docs/specs/381-no-op-tap-action.md` и `docs/specs/README.md` — + продуктовый код не тронут (закономерно для этапа spec) +- Заход: r1 · блокирующих циклов израсходовано **0 из 4** до этого вердикта + +## Скоуп ревью + +Первый заход — предыдущего вердикта нет, разбор полный. Проверялось: + +- соответствие `docs/SCOPE.md` (job, персона, поверхность); +- корректность выбора трека (полный, не `small`/`trivial`) по критериям §5; +- наличие всех обязательных разделов ТЗ по §7.1, включая «риски» и «откат»; +- однозначность и доказуемость каждого AC1…AC9, указание способа + доказательства; +- отсутствие догадок, выданных за факт: каждое утверждение ТЗ о текущем + поведении кода сверено с исходниками, а не принято на слово; +- согласованность терминологии с `docs/USER-GUIDE.ru.md` и `TOUCH-SUPPORT.md`; +- полнота списка «Затронутые файлы и модули» — не пропущен ли реальный + потребитель `tap_action`. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `PROCESS.md` (§2, §4, §5, §7, §8), `AGENTS.md`. +2. Прочитано тело issue #381 и все три комментария (аналитика, claim, + публикация ТЗ) через `gh issue view --json`. +3. Прочитан ТЗ `docs/specs/381-no-op-tap-action.md` целиком. +4. Каждое фактическое утверждение ТЗ о текущем коде сверено с `dev`: + - `src/logic.ts:910` — `TAP_ACTIONS = ['info', 'more-info', 'toggle', + 'run']` — подтверждено, ровно четыре текущих действия. + - `src/device-toggle.ts:463-475` (`projectedTapAction`) — подтверждено: + любой нераспознанный непустой токен возвращает `'info'` («fail closed»), + проекция света по умолчанию срабатывает только при реальном отсутствии + значения. + - `src/houseplan-card.ts:5187-5313` (`_clickDevice`) — подтверждено: после + веток `toggle`/`info`/`run`/`more-info` функция заканчивается безусловным + `this._infoCard = actionDevice;` — реальный fallback на локальную + карточку, который контракт §2.2 обязан обойти до него, а не после + (проверен соответствующий пункт мутантов). + - `src/houseplan-card.ts:5325-5330` (`_keyDevice`) — подтверждено: + `Enter`/`Space` вызывают тот же `_clickDevice()` после `preventDefault()`. + - `src/houseplan-card.ts:5178-5185` (`_ctxDevice`) и long-press на 600 мс + (`:6360-6364`) — подтверждено: оба жеста не читают `tap_action` вообще, + независимы от выбора действия, как заявляет контракт §3. + - `src/houseplan-editor-runtime.ts:12346` — маппинг селектора + `TAP_ACTIONS.map((v) => [v, 'tap.' + v.replace('-', '_')])` — подтверждено, + что `none` автоматически получит ключ `tap.none` без спецкода. + - `src/houseplan-editor-runtime.ts:12337,12352,12388,12394,12423` — + подтверждено: Toggle-hint, target chooser и confirmation-чекбокс скрыты + условием `effectiveTapAction === 'toggle'/'run'`; для `none` они погаснут + автоматически, без новой ветки видимости. + - `src/houseplan-editor-runtime.ts:7934-7940` (`_markerTapActionFields`) — + подтверждено: untouched-путь (`!tapActionTouched`) пишет + `originalTapAction` как есть, включая legacy/неизвестный литерал — + lossless-контракт AC5 уже существует для этого поля, задача его не ломает. + - `src/houseplan-editor-runtime.ts:8034` — `tap_target` пишется `null` для + любого `effectiveTapAction !== 'run'` — подтверждает пункт «Принятых + предположений» про очистку `tap_target`. + - `custom_components/houseplan/validation.py:1693` — `MARKER_SCHEMA` + принимает `vol.Any("info", "more-info", "toggle", "run", "cover", None)` — + подтверждено, backend действительно отклонит седьмой (`none`) без + schema-правки. + - `custom_components/houseplan/import_export.py:1386` — подтверждён список + полей, которые virtualization дубликата стирает вместе с `tap_action`. + - `src/device-presentation.ts:264-338` — подтверждено, что визуальная роль + cover-устройства (`coverOwnsFace`) не зависит от выбора `none`: код уже + форсирует `tapAction: 'cover'` для presentation independent от + персистентного действия, поэтому AC4 (presentation parity) не требует + здесь правок. + - `src/i18n/{en,ru,de,fr}.json` — подтверждено, что это ровно четыре + существующих словаря (пятого/шестого локале нет) и что паттерн ключей + `tap.` уже используется (`tap.info`, `tap.toggle`, `tap.run`). + - `tests_backend/test_validation.py` — подтверждено существование + cross-language parity теста `TAP_ACTIONS`, на который ссылается план + автотестов (это не выдуманная будущая инфраструктура). +5. Сверена терминология: `docs/USER-GUIDE.ru.md:812` подтверждает, что «Действие + по нажатию» — существующее название поля редактора, не изобретённое ТЗ. +6. `docs/TOUCH-SUPPORT.md` — подтверждено, что View/kiosk — гарантированно + touch-поверхность; ТЗ явно требует паритет mouse/touch/keyboard в AC2, что + закрывает блокирующее требование §2.5 без отдельного раздела «Touch». +7. Найден прямой прецедент калибровки строгости в этом же репозитории: + `docs/reviews/SPEC-REVIEW-376-r1.md` (M1 «раздел откат отсутствует + полностью» — классифицирован как **Medium**, не High) и + `docs/reviews/SPEC-REVIEW-377-r1.md` (тот же тип находки, вердикт + `жёлтый · High: 0 · Medium: 1`). Severity ниже откалибрована по этому + прецеденту, а не назначена заново. +8. Сравнены с шаблоном двух последних full-track ТЗ этого же автора — + `docs/specs/377-decor-default-persist.md` и `docs/specs/378-value-face-source.md` + — оба содержат отдельные разделы `## Риски` и `## Откат`; это подтверждает, + что раздел ожидается предметно, а не просто присутствует в §7.1 формально. + +Гейты (`tsc`/`test`/`build`/`invariants`/smoke/golden) не прогонялись: этап — +ревью ТЗ, продуктовый код в этой ветке не менялся (см. `git diff --stat` выше). +Они относятся к этапу код-ревью (§2.7) и будут актуальны в следующем цикле. + +## Находки + +### M1 (Medium, в скоупе) — отсутствуют обязательные разделы «Риски» и «Откат» (§7.1) + +`docs/specs/381-no-op-tap-action.md` не содержит `## Риски` и `## Откат`. +Оба раздела прямо названы обязательными в PROCESS.md §7.1 («Обязательные +разделы ТЗ: … план автотестов · **риски** · **откат** · release-артефакты») и +входят в чек-лист DoR §2.5 («риски перечислены», «откат: как выключить или +вернуть назад»). Оба непосредственно предшествующих full-track ТЗ этого же +автора — `docs/specs/377-decor-default-persist.md` и +`docs/specs/378-value-face-source.md` — оба раздела содержат с предметным +наполнением (конкретные риски со снимающим механизмом; `git revert` + +анализ потери данных для отката). В 381 часть содержания фактически +рассеяна по тексту («Известная граница downgrade» в разделе Import/export, +раздел «Мутанты» как список технических рисков тестирования), но ни то, ни +другое не отвечает на вопрос §2.5 буквально: как откатить именно этот выпуск, +если `none` после релиза окажется проблемным (флага Labs нет, отдельной +миграции нет — значит ответ, вероятно, тривиален: `git revert` +реализационных коммитов, уже записанный `tap_action: "none"` остаётся в +конфиге и безопасно читается как `info` старым фронтом — но это нужно +написать явно, а не оставлять читателю выводить самому), и не перечисляет +предметные риски конкретно этого контракта (например: неполный список +потребителей `tap_action` в «Затронутые файлы» — риск того, что найдётся +шестой потребитель уже после реализации; путаница `none` vs `null` в ручном +редактировании YAML/API в обход UI). + +**Воспроизведение:** `grep -n "^## Риски\|^## Откат" docs/specs/381-no-op-tap-action.md` +не находит ни одного совпадения; `grep -n "^##"` (полный список выше в +разделе «Как проверялось» не повторяю) показывает переход прямо от «План +автотестов» к «Производительность и безопасность» и «Release-артефакты». + +**Серьёзность откалибрована по прецеденту.** `SPEC-REVIEW-376-r1.md` нашёл +структурно идентичную находку («раздел «откат» отсутствует полностью») и +классифицировал её как Medium, а не High; красный вердикт того раунда был +вызван отдельной, не связанной High-находкой. Здесь High-находок нет, поэтому +по правилу §2.4 («без High это жёлтый вердикт») этот Medium один определяет +вердикт «жёлтый». + +**Что делать:** добавить `## Риски` (2–4 пункта: неполный список потребителей +`tap_action`, путаница `none`/`null` в ручном редактировании конфига в обход +UI, что ещё именно автор считает риском этого контракта) и `## Откат` (git +revert реализационных коммитов; судьба уже записанного `tap_action: "none"` +на старом/новом фронте и бэкенде — эта часть уже фактически написана в разделе +«Import/export и downgrade», её нужно только процитировать/сослаться под +правильным заголовком, а не сочинять заново). + +### L1 (Low, снимается) — AC4 называет способ доказательства словом вне канонического словаря + +AC4 помечен «unit + visual assertion»; §2.5 перечисляет канонические ярлыки +`unit`/`backend`/`smoke`/`golden`/«ревью кода». «Visual assertion» не входит +в этот список и по названию мог бы намекать на golden-скриншот, которого +раздел «Release-артефакты» этого же ТЗ прямо исключает («Новый plan golden не +требуется…»). Снимаю без правки: раздел «Release-артефакты» и формулировка +AC4 («совпадают DOM/classes/…») однозначно указывают, что имеется в виду +DOM/class-assertion в unit-тесте, а не отдельный вид гейта — противоречия по +существу нет, только словарь. Если автор всё равно правит документ по M1, +имеет смысл переименовать в «unit (DOM assertion)» заодно, но отдельного +цикла это не требует. + +## Что проверено и корректно + +- **Продуктовая рамка.** Job **J3** `docs/SCOPE.md` — точный якорь; пассивный + маркер как явный выбор администратора не противоречит ни одному пункту + «Out of scope» и не расширяет action-контракт за пределы короткого клика. + Сценарий и «что человек увидит» — конкретные, без терминов реализации, + отвечают на оба обязательных вопроса §7.1. +- **Трек выбран верно.** Аргумент «новый UX-контракт» из аналитики + подтверждён кодом (новый persisted enum literal, новое наблюдаемое + поведение) — не small и не trivial. +- **Все технические утверждения о текущем коде подтверждены чтением + исходников** (полный список — раздел «Как проверялось», пункт 4); догадок, + выданных за факт, не найдено. +- **Контракт поведения однозначен и без домыслов**, включая точку вставки + no-op в `_clickDevice()` (до fallback-карточки, а не после) — контракт + явно называет актуальный порядок веток и совпадает с реальным кодом. +- **AC1…AC9 проверяемы**, у каждого указан способ доказательства; AC7 + (backend) корректно требует cross-language parity test, который уже + существует и будет расширен, а не написан с нуля. +- **План автотестов и раздел «Мутанты»** называют конкретные ошибки + реализации (спроецировать `none` в `info`; вставить no-op после + fallback-карточки; связать `none` с недоступным Toggle) — это ровно те + ошибки, которые нашлись бы при поверхностном прочтении текущего кода, + что показывает, что план тестов написан против реального риска, а не + формально. +- **i18n и терминология** — «Действие по нажатию» и паттерн ключей `tap.*` + взяты из существующего кода/документации, не изобретены; ровно 4 словаря + названы и ровно 4 существуют. +- **Touch и defaults** решены и не оставлены открытыми: паритет + mouse/touch/keyboard явно в AC2, defaults явно не меняются (Скоуп/Не-скоуп). +- **Release-артефакты** называют оба changelog в скоупе одного коммита с + реализацией — соответствует политике `User-Visible: yes` (§2.6). +- **Открытых продуктовых вопросов действительно нет**: единственная зона + неоднозначности («марker остаётся видимым, без disabled-вида») напрямую + закрыта формулировкой самого issue («Выбранный маркер остаётся видимым и + показывает свои состояния») — это не догадка автора ТЗ, а прямая цитата + владельца. + +## Чего не проверял + +- Не прогонялись `npx tsc --noEmit`, `npm test`, `npm run build`, + `node scripts/check-docs.mjs`, `npm run invariants`, браузерные смоки и + `golden:verify` — на этапе ревью ТЗ они неприменимы: продуктовый код не + менялся (см. `git diff --stat` в шапке документа). Они станут обязательны + на этапе код-ревью (§2.7). +- Не проверялась реализация (её нет) — весь разбор выше про код относится к + **текущему**, немодифицированному `dev` и служит только проверке того, что + фактические утверждения ТЗ о текущем поведении верны. +- Не проверялась орфография/стиль переводов DE/FR по существу (нет статуса + носителя языка) — только структурное наличие всех четырёх ключей и + соответствие паттерну именования. + +## Вердикт + +Один Medium в скоупе, High нет → по §2.4 жёлтый вердикт, возврат автору на +доработку двух обязательных разделов; остальной контракт (AC, модель данных, +i18n, touch, defaults, release-артефакты) готов и не требует повторного +разбора, если правка ограничится добавлением «Риски»/«Откат» (это будет +проверено по дельте в r2, PROCESS.md §2.9/§2.10). + +`Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 1 → в задаче · Документ: docs/reviews/SPEC-REVIEW-381-r1.md`