diff --git a/docs/reviews/SPEC-REVIEW-103-r1.md b/docs/reviews/SPEC-REVIEW-103-r1.md new file mode 100644 index 00000000..1c89fc9e --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-103-r1.md @@ -0,0 +1,257 @@ +# SPEC-REVIEW-103-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/103 +- **ТЗ под ревью:** `docs/specs/103-toggle-confirmation-state.md` (коммит + `49c0c3d`, ветка `issue/103-toggle-confirmation-state`) +- **Роль:** ревьюер ТЗ (не автор), этап `S4-spec-review` +- **Трек:** обычный (не `small`/`trivial`) — согласно аналитике владельца от + 2026-08-14: сложность 2/10, риск 3/10, но `trivial` намеренно не применён, + так как confirmation copy — UX-контракт, потенциально затрагивающий + несколько доменов (power/cover/valve/group/virtual light) +- **Цикл:** r1/4 + +## Скоуп ревью + +Проверялось соответствие ТЗ: + +- `docs/SCOPE.md` — попадание задачи в Core user jobs, отсутствие расширения + скоупа и конфликта с «lock invariant»; +- `PROCESS.md` §2.4, §2.5 (DoR), §7.1 (обязательные разделы), §3/§12 + (запреты), §5 (легкий трек — не применяется, но проверено, что признаки + `small` действительно отсутствуют); +- `AGENTS.md` — классы файлов коммита, ветка, трейлеры; +- фактическому состоянию кода `src/device-toggle.ts` и `src/houseplan-card.ts` + — технические утверждения ТЗ о существующем резолвере #94 + (`ResolvedToggleIntent`, `formatToggleIntent`, `sameToggleOperationTargets`, + `_tapConfirm`) сверены построчно, чтобы отличить факт от догадки; +- `docs/USER-GUIDE.ru.md` — терминология («Переключить состояние», hint под + селектором действия, `tap_confirm`); +- `docs/TOUCH-SUPPORT.md` — категория «View dialogs and safe device actions» + (fully supported на touch, не best-effort). + +## Как проверялось + +1. Прочитан весь тред issue #103: исходное тело (уже содержит нормативную + матрицу, i18n-ключи и race-контракт от владельца), комментарий аналитики + 2026-08-15 (`P3`, сложность 2/10, риск 3/10, «вопросов нет»), комментарий + автора ТЗ («продуктовых вопросов нет, метка остаётся `S3-spec` по поручению + владельца» — на момент ревью метка уже `S4-spec-review`, расхождение не + процессное: `docs/specs/README.md` подтверждает коммит и ветку). +2. Сверены обязательные разделы ТЗ (§7.1 PROCESS.md) построчно — таблица ниже. +3. Прочитан `src/device-toggle.ts` целиком: подтверждено существование + `ResolvedToggleIntent`, `ToggleNextEffect`, `formatToggleIntent()`, + `sameToggleOperationTargets()`, `toggleOperation()` — ровно тот резолвер + #94, на который ссылается ТЗ, а не придуманный интерфейс. +4. Прочитан `src/houseplan-card.ts` (окрестности `_clickDevice`, `:4336-4400` + и `:14838-14846`): подтверждено дословно, что текущий `_tapConfirm` — + `{ text: string; exec: () => void }`, а `text` строится только как + `this._t('confirm.tap_toggle', { name })` — то есть претензия ТЗ «диалог + показывает только имя» верна для текущего кода, не выдумана. +5. Прочитан `_toggleHintLines()` (`:17314-17358`) и соответствующие ключи + `marker.toggle_hint_current` / `marker.toggle_effect_*` / + `marker.toggle_hint_group_current` / `marker.toggle_hint_skipped` в + `src/i18n/ru.json` и `en.json`. Эта существующая инфраструктура уже строит + «текущее → эффект» для редакторского hint под селектором действия + (`docs/USER-GUIDE.ru.md:489`). ТЗ не игнорирует её: §5 явно допускает + расширение `formatToggleIntent()` либо добавление соседней pure-функции, а + разделение current/expected на отдельные строки в §8 обосновано + собственным accessibility-требованием (порядок чтения screen reader'ом), + которого нет у однострочного hint. Дублирования семантики без причины не + обнаружено. +6. Проверена логика `coverLikeService()` (`:307-331`) против нормативной + матрицы §6 ТЗ — см. Low-3. +7. Прочитан `test/device-toggle.test.mjs` и список `demo/smoke_*.mjs`: + подтверждено существование `smoke_ha_controls.mjs`, `smoke_controls.mjs`, + `smoke_virtual_light_toggle.mjs`, на которые ссылается план регрессии §11 — + не выдуманные имена. +8. Прочитан `docs/TOUCH-SUPPORT.md`: строка «View dialogs and safe device + actions» — «Fully supported» на touch (не best-effort), то есть touch- + влияние здесь блокирующее по DoR; ТЗ §8 адресует это узкой mobile-footer и + narrow-viewport smoke — соответствует канону. +9. Прочитан `docs/USER-GUIDE.ru.md:489` и «Краткая памятка безопасности» + (:1206): термин «Переключить состояние» и рекомендация включать + подтверждение для Toggle/Run/Cover совпадают с ТЗ дословно. +10. Проверена трассируемость: `docs/specs/README.md` получил секцию `## P3` + со строкой на #103 в том же коммите; `git diff --stat origin/dev...HEAD` + показывает только два файла класса C; коммит `49c0c3d` несёт + `Issue: #103` и `User-Visible: no` — корректно для документа ТЗ, который + сам не меняет поведение. + +## Обязательные разделы (§7.1 PROCESS.md) + +| Раздел | Есть | Комментарий | +|---|---|---| +| Сценарий (персона/поверхность/момент) | Частично | §1 объясняет проблему технически (только имя в диалоге), но ни разу не называет персону/поверхность явной фразой — см. Low-1 | +| Что человек увидит до/после | ✅ | §2, буквально в виде текста диалога «до/после» | +| Проблема | ✅ | §1 | +| Скоуп / не-скоуп | ✅ | §3 / §4 | +| Контракт поведения | ✅ | §5, §6, §9 | +| Модель данных и миграция | ✅ | §7 (TS-интерфейс `_tapConfirm`) + §9 («config не меняются», миграции нет) | +| UX и i18n | ✅ | §8 | +| AC1…ACn с доказательством | Частично | §10 — 10 пронумерованных AC, но без построчной метки типа доказательства; тип восстанавливается однозначно из §11 (см. Low-2) | +| План автотестов | ✅ | §11, разбит на unit/integration/регрессию | +| Риски | ✅ | §14 | +| Откат | ✅ | §14 (последний абзац) | +| Release-артефакты | ✅ | §13 | + +Дополнительно есть раздел «Принятые технические предположения» (§15) — +не требуется §7.1 буквально, но прямо соответствует духу PROCESS.md §7.1 о +записи технических решений отдельным блоком. + +## Находки + +Находок уровня **High** и **Medium** нет — отдельные issue заводить не +требуется. + +### Low-1 — сценарий не называет персону/поверхность явной фразой + +**Файл:** `docs/specs/103-toggle-confirmation-state.md` (§1) + +AGENTS.md требует, чтобы первый раздел ТЗ отвечал: какая персона (из +`docs/SCOPE.md`), на какой поверхности, в какой момент встретит изменение. +§1 объясняет техническую проблему («текущий диалог показывает только имя»), +но не пишет explicit: диалог tap-confirm — это «View dialogs and safe device +actions» (`TOUCH-SUPPORT.md`), доступный любой персоне на любой поверхности +(desktop/kiosk/companion app) в момент нажатия на маркер с `tap_confirm`. +Смысл восстанавливается из §2 (пример текста диалога) и общего контекста +J3 `SCOPE.md`, поэтому неоднозначности для читателя нет, но формальное +требование выполнено не буквально. + +**Решение ревьюера:** Low, не блокирует. Косметическая правка одним +предложением на усмотрение автора при следующей редакции. + +### Low-2 — AC1–AC10 не промаркированы построчно типом доказательства + +**Файл:** `docs/specs/103-toggle-confirmation-state.md` (§10, AC1–AC10) + +DoR (`PROCESS.md` §2.5) и цепочка §7.1 требуют, чтобы «у каждого [AC] указано, +чем он доказывается: unit / backend / smoke / golden / ревью кода». §10 +перечисляет десять проверяемых критериев, но ни один не несёт явной метки +доказательства — план тестирования (§11) даёт эти метки только группами +(Unit / Integration-browser / Регрессия), и сопоставление 1:1 нужно +восстанавливать вручную. Я проверил это сопоставление вручную: AC1–AC6 +однозначно закрываются перечисленными в §11 unit-кейсами (formatter per +`ToggleNextEffect`, single/group/partial/unknown/no-operation), AC7–AC8 — +race-кейсами из Integration/browser, AC9 — DOM order/narrow viewport/keyboard +smoke, AC10 — «run confirmation unchanged» той же секции. Ни один AC не +остаётся недоказуемым по существу; отсутствует только явная метка в самом +тексте §10. Тот же класс находки (типы доказательства AC вне буквального +формата) уже фиксировался как Low и не блокировал приёмку в +`SPEC-REVIEW-137-r1` (Low-3). + +**Решение ревьюера:** Low, не блокирует. Рекомендуется при следующей правке +приписать к каждому AC1–AC10 короткую метку (`unit`/`integration`/`review +кода`), чтобы код-ревью могло сверяться механически, а не восстанавливать +сопоставление заново — но проверяемость критериев уже подтверждена этим +документом. + +### Low-3 — нормативная матрица §6 группирует cover-состояния не так, как резолвер + +**Файл:** `docs/specs/103-toggle-confirmation-state.md:78-79` (§6, строки +«cover `closed/closing` + `open`» и «cover + `stop`») +**Код:** `src/device-toggle.ts:307-331` (`coverLikeService`) + +Таблица группирует «closed/closing» в одну строку с ожидаемым `open`. Но по +факту резолвера состояние `closed` действительно даёт `nextEffect: 'open'`, +а `closing` — отдельную ветку `nextEffect: 'stop'` (наравне с `opening`), +то есть реальная комбинация «`closing` → next `open`» в резолвере никогда не +возникает: она относится к строке «cover + `stop`» этой же таблицы. Так как +формула §5 явно требует брать current-строку из HA-formatted state, а не +пересчитывать её из этой таблицы, и направление всегда берётся из +`nextEffect` резолвера (а не выводится реализацией по названию строки), риска +для рантайма нет — но группировка вводит в заблуждение при написании unit- +теста по этой таблице буквально (можно ошибочно закодировать несуществующую +комбинацию `state=closing + effect=open` как «нормальный» кейс вместо +`state=closing + effect=stop`). + +**Решение ревьюера:** Low, не блокирует. При следующей редакции стоит убрать +`closing` из строки `open` (оставить только `closed`) и явно отметить, что +строка `stop` покрывает оба переходных состояния (`opening`/`closing`) — но +это не меняет ни один AC и не требует возврата цикла. + +## Что проверено и корректно + +- Соответствие `docs/SCOPE.md`: задача уточняет содержимое существующего + Toggle-confirmation guard'а — это J3 («Let me act on the obvious right from + the plan», guarded quick actions), статус **Closed**; расширения продукта + нет, задача улучшает уже принятую функциональность и не создаёт нового + actuation-пути. Явно проверено, что «lock invariant» `SCOPE.md` не задет: + secure-цели (`lock.*`, `alarm_control_panel.*`, guarded cover) уже + фильтруются резолвером на уровне `secureEntity()` и никогда не попадают в + `targets`, следовательно новый confirmation-текст не может *обещать* + переключение замка. +- Легитимность полного трека: несмотря на низкую оценку сложности (2/10) от + владельца, автор корректно не применил `trivial`/`small` — confirmation + copy реально затрагивает пять разных семантик (power/cover/valve/group/ + virtual light) и i18n; критерий «одна поверхность» (§5 PROCESS.md) не + выполняется буквально, обычный трек оправдан. +- Продуктовых вопросов владельцу нет, и это корректно: почти вся нормативная + матрица уже была продиктована владельцем в теле issue до написания ТЗ, + автор её только формализовал и уточнил (см. Low-3 как единственное реальное + уточнение, не вопрос). Ни одна догадка не выдана за факт без пометки — + §15 отдельно фиксирует три технических предположения (no live-update + snapshot, HA-formatter приоритет, `stop` как честный ожидаемый эффект), + все они соответствуют либо явному тексту issue, либо существующему коду. +- Технические утверждения о текущем коде подтверждены чтением, а не + голословны: `ResolvedToggleIntent`/`formatToggleIntent`/ + `sameToggleOperationTargets` (#94, `src/device-toggle.ts`), текущий + однострочный `_tapConfirm.text` (`src/houseplan-card.ts:4388-4389`), + существующий toast `toast.tap_target_changed`, существующие тесты + `test/device-toggle.test.mjs` и смоки `smoke_ha_controls.mjs` / + `smoke_controls.mjs` / `smoke_virtual_light_toggle.mjs`. +- Не-скоуп (§4) корректно отсекает смежные соблазны: изменение resolver/ + target-selection/service-call, прогноз scripts/scenes, история состояний, + redesign confirmation-диалогов удаления/unlock/run, live-анимация в + открытом диалоге, изменение схемы `tap_confirm` — типичные места, где скоуп + мог бы незаметно расшириться, явно исключены. +- Touch-контракт: диалог tap-confirm — категория «View dialogs and safe + device actions» (`TOUCH-SUPPORT.md`), полностью поддерживаемая на touch + (не best-effort). ТЗ §8 адресует это явно (narrow footer с двумя кнопками, + перенос длинного текста без horizontal scroll) и §11 добавляет narrow- + viewport smoke — соответствует блокирующей категории канона. +- Совместимость: `stored tap_confirm` и схема config не меняются, миграции + нет — корректно для confirmation copy, не меняющей контракт хранения. + Откат (§14) корректно опирается на то, что `_tapConfirm` возвращается к + одной строке без отдельного data rollback. +- i18n: ключи перечислены раздельно для RU/EN в одном release-коммите (§13); + plural rules осознанно не вводятся, что соответствует «counter-safe style» + уже принятому в существующих `marker.toggle_*` строках. +- Release-артефакты (§13) корректно относят golden/screenshot к точечному + необязательному кадру, а backend/migration/performance/security — явно + «не требуются», что соответствует характеру изменения (чистый UI-текст без + новых сервис-вызовов). +- Трассируемость: `docs/specs/README.md` обновлён тем же коммитом (новая + секция `## P3` со строкой на #103), ссылка issue ↔ ТЗ двусторонняя; ветка + `issue/103-toggle-confirmation-state` и трейлеры коммита (`Issue: #103`, + `User-Visible: no`) корректны для документа класса C, который сам не + меняет поведение. +- `git diff --stat origin/dev...HEAD` не содержит ни одного файла класса A — + код не тронут до `S5-ready`, что соответствует правилу №1. + +## Чего не проверял + +- Не проверял реализуемость `_tapConfirm` как размеченного union + (`{kind:'toggle', ...} | {kind:'run', ...}`) на уровне TypeScript-типов — + это по правилам ТЗ свободно изменяемое техническое решение автора кода + (§15), не предмет ревью ТЗ. +- Не запускал автотесты, build или browser-смоки — на этапе `spec` это не + требуется; существование резолвера, hint-инфраструктуры, i18n-ключей и + тестовых/смоук-файлов, на которые ссылается ТЗ, проверено чтением + исходников, а не исполнением. +- Не проверял корректность численных оценок аналитики (ценность 5/10 и 3/10, + сложность 2/10, риск 3/10, P3) по существу — это поле владельца + (PROCESS.md §2.2), уже принятое явным решением до написания ТЗ. +- Не проверял, как именно `hass.formatEntityState` поведёт себя для каждого + конкретного домена/state на реальном HA — ТЗ корректно требует safe + fallback на raw state, и это уже проверяемый unit-кейс по плану §11, а не + вопрос ревью ТЗ. + +## Вердикт + +Зелёный. High: 0, Medium: 0. Три находки Low (сценарий не называет +персону/поверхность явной фразой; AC1–AC10 не промаркированы построчно типом +доказательства, хотя проверяемость подтверждена этим документом; нормативная +матрица §6 вводит в заблуждение группировкой cover `closed/closing` в строке +`open`, хотя резолвер направляет `closing` в строку `stop`) — ни одна не +блокирует приёмку и не меняет ни один AC; все три либо правятся косметически +при следующей редакции, либо снимаются этой записью без нового цикла.