diff --git a/docs/reviews/SPEC-REVIEW-212-r1.md b/docs/reviews/SPEC-REVIEW-212-r1.md new file mode 100644 index 00000000..7b32a1db --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-212-r1.md @@ -0,0 +1,182 @@ +# SPEC-REVIEW-212-r1 + +- **Issue:** [#212](https://github.com/Matysh/houseplan-card/issues/212) — «Правки по новым иконкам» +- **ТЗ:** `docs/specs/212-device-icons-polish.md` (обычный трек, не `small`) +- **Этап:** ревью ТЗ (PROCESS.md §2.4) +- **Вердикт:** зелёный +- **Цикл:** r1/4 +- **High:** 0 · **Medium (в скоупе):** 0 · **Medium (вне скоупа):** 0 · **Low:** 1 (снят с записью) + +## Скоуп ревью + +Issue #212 — консолидация трёх открытых задач (#22 отклик на нажатие, #154 +sticky touch-hover, #181 неверный адрес документации) плюс собственные пункты +владельца: уменьшение маркера устройства на 10% и исправление формы капсулы +`display: value`. ТЗ живёт файлом `docs/specs/212-device-icons-polish.md` +(трек «обычный», не `small`) — формат файла корректен для этого трека. + +Проверялось: тело и комментарии #212, тела #22/#154/#181 (полностью, без +усечения), #179 и #211 (нормативный дизайн-пакет и его принятое исправление), +`docs/SCOPE.md`, `docs/TOUCH-SUPPORT.md`, `docs/UX-MODES.md`, +`docs/CONFIG-COMPATIBILITY.md`, сам текст ТЗ целиком (§1–§22) и — построчно — +код на `dev`, который ТЗ цитирует как «подтверждённую причину» в §3 и как +основание архитектурных решений в §16. + +## Как проверялось + +Ревью на этапе `spec` не запускает гейты сборки/тестов — предмет проверки +это ambiguity, доказуемость AC и отсутствие выданных за факт догадок. Вместо +прогона тестов я построчно сверил каждое фактическое утверждение ТЗ о текущем +коде с самим кодом на `dev` (`ec6f77b`), поскольку §3 ТЗ прямо заявляет +«аудит текущего dev подтвердил четыре независимых дефекта» — такое заявление +обязано быть проверяемым, а не риторическим. + +Сверено чтением (`grep`/`sed` по `src/styles.ts`, `src/houseplan-card.ts`, +`src/space-card.ts`, `src/hp-dialog.ts`, `src/hp-help.ts`, +`src/hp-color-opacity.ts`, `src/vacuum.ts`): + +| Утверждение ТЗ | Где | Результат сверки | +|---|---|---| +| §3.1: shell = `1.26875 × core`, нет отдельного коэффициента 0.9 | `styles.ts:1877` | подтверждено; `grep` на `* 0.9` / `--global-icon-factor` — ноль совпадений | +| §3.2: `.device-core` держит `border-radius: 50%` глобально, `valonly` растит только ширину | `styles.ts:1812–1816, 1991–2001` | подтверждено — эллипс на широком value неизбежен | +| §3.2: shell использует постоянный радиус `device-shell-size/2` | `styles.ts:1939, 1955` | подтверждено | +| §3.3: `_clickDevice()` не создаёт transient feedback | `houseplan-card.ts` (нет отдельного feedback-owner рядом с dispatch) | подтверждено отсутствием | +| §3.4: `_touchSeen` — статический (глобальный) флаг, `_notePointer()` закрывает только `_tip` | `houseplan-card.ts:5765,5776–5779` | подтверждено | +| §3.4: `_hoverRoom` пишется из `mouseenter`/`mouseleave` | `houseplan-card.ts:15837–15864` | подтверждено | +| §3.5/#181: hover-контракт View описан в `UX-MODES.md`, не в `CANVAS.md` | `UX-MODES.md` «View — display and device interaction only» | подтверждено дословно | +| §7.3: Double-value pill уже имеет константу `0.39375 × core` | `styles.ts:2248` | подтверждено | +| §7.2: vacuum puck зависит от `--dev-size` | `styles.ts:2556` (`--puck-size: calc(var(--dev-size)…)`) | подтверждено | +| §16: перечисленные файлы (`device-face.ts`, `space-render.ts`, `hp-device-preview.ts`, `hp-dialog.ts`, `hp-help.ts`, `hp-color-opacity.ts`, `space-card.ts`) существуют; `pointer-modality.ts` — новый | `ls src/` | подтверждено | +| §13: единственный `:hover` в `space-card.ts` — `.hp-static-btn:hover` | `space-card.ts:903` | подтверждено | +| §13: по одному `:hover` в `hp-dialog.ts`/`hp-help.ts`/`hp-color-opacity.ts` (`.close`, `.trigger`) | построчный `grep` | подтверждено | +| §19.1: названные smoke-файлы существуют, «device action feedback smoke» — новый, не переиспользование `smoke_feedback_v2.mjs` | `ls demo/`, чтение `smoke_feedback_v2.mjs` | подтверждено; `smoke_feedback_v2.mjs` вообще не про device-маркеры (комнатная gear-кнопка) — ТЗ верно не сослалось на него | +| §4: версия пакета «1.1.1» | `docs/specs/179-…md`, `docs/specs/211-…md`, `docs/TESTING.md:342` | подтверждено предыдущими принятыми ТЗ, не изобретено | +| 44×44 CSS px как уже существующий пол, а не новое требование | `styles.ts:1939` (`max(44px, var(--device-shell-size))`) | подтверждено | + +Все проверенные фактические утверждения ТЗ о текущем состоянии кода и +документации оказались точными. Раздел §3 «Проблема и подтверждённая причина» +не содержит ни одной догадки, выданной за факт, — каждый пункт воспроизводится +в реальном коде по указанным строкам. + +Дополнительно сверены структурные требования PROCESS.md §7.1: сценарий, +«что человек увидит», проблема, скоуп/не-скоуп, поведенческий контракт, UX, +данные/миграция/i18n, AC1…AC16 с указанием способа доказательства, план +тестов, риски, откат, release-артефакты — все разделы присутствуют, ни один +не сведён к формальной заглушке. Блок §22 «принятые технические +предположения» оформлен явно и содержит только технические (не продуктовые) +решения — ни один из семи пунктов не подменяет продуктовое решение техническим +языком. + +## Находки + +### Low-1 — межевой случай «plan-wide setting» из #22 закрыт молча, без явной пометки + +- **Файл:** `docs/specs/212-device-icons-polish.md`, §6 «Не входит в задачу» +- **Что происходит:** оригинальный текст #22 (полностью поглощаемого этим ТЗ) + перечисляет ожидаемое поведение как «a subtle scale dip or flash, + **plan-wide setting**, only on devices that actually act, reduced-motion + fallback». Формулировка «plan-wide setting» синтаксически допускает два + прочтения: (a) эффект применяется единообразно по всему плану, без + per-marker переопределения — то, что фактически описывает ТЗ в §10; или + (b) запрос на новый пользовательский переключатель уровня плана + (вкл/выкл или интенсивность). ТЗ выбирает (a) молча и одновременно явно + исключает (b) в §6 («новый пользовательский toggle интенсивности/ + длительности feedback»), не фиксируя, что здесь вообще была развилка. +- **Почему не Medium.** Это ровно тот тип вопроса, который по PROCESS.md + §7.1 адресуется владельцу («какой объём видимых изменений входит в этот + issue»), но: (1) §4 ТЗ прямо и обоснованно ставит текст #212 выше текста + #22 по приоритету, а сам #212 (написанный позже, тем же владельцем) + перечисляет пять конкретных пунктов и ни словом не упоминает настройку; + (2) §15 ТЗ уже держит открытым предохранитель — если реализация всё же + потребует новую строку/переключатель, это автоматически трактуется как + расширение скоупа с отдельным решением владельца, то есть риск «тихо + потерять требование навсегда» отсутствует. +- **Решение ревьюера:** снимается с записью. Автору стоит одной строкой в + §22 зафиксировать, что «plan-wide setting» прочитано как «единая политика + на весь план, не per-marker», а не как новый UI-переключатель — это + ничего не меняет по существу, но убирает тихое разночтение источника, + которое иначе выглядит как то самое «оставили в тексте, а не решили». + +Других находок — ни High, ни Medium, ни дополнительных Low — не выявлено. + +## Что проверено и корректно + +- **Формат трека.** Issue не помечен `small`/`trivial` (сложность и риск 8/10 + по аналитике владельца) — файл в `docs/specs/` обязателен и создан + корректно, `small`-путь неприменим. +- **Продуктовые разделы.** §1 «Сценарий» называет персону из `docs/SCOPE.md` + (домочадец на телефоне/панели/компьютере, админ в редакторе устройств) и + момент («нажимает на устройство»); §2 «Что человек увидит» — одной парой + фраз, без терминов реализации (не упоминает `--dev-size`, resolver, + pointermodality и т.п.). +- **Обоснование скоупа по SCOPE.md.** Задача закрывает J1/J2/J7 (спойлер + плашки для длинного значения читается хуже — J1; отклик на нажатие — J3; + честный touch-контракт — обязательное требование `TOUCH-SUPPORT.md`, а не + отдельная строка SCOPE, но сам документ называет touch-only дефект View + продуктовым багом). Ничего из «Out of scope» SCOPE.md не задето: цвета, + lock-инвариант, glyph viewport, LQI threshold — везде явно исключены в §6. +- **Lock-инвариант не тронут.** §6 явно исключает изменение lock security; + §9 явно исключает feedback для «secure/no-target/unavailable/missing/ + unsupported no-op»; confirmation-путь (§9) требует повторной валидации + target при подтверждении — согласуется с параграфом «The lock invariant» + `docs/SCOPE.md` (единственная санкционированная поверхность — кнопка + Unlock/Lock внутри открытой карточки, а не тап по плану). +- **Правильный канонический адрес документации.** §19.4 обновляет + `docs/UX-MODES.md` (не `docs/CANVAS.md`) для hover/pressed-владения — + именно то, что требует #181, с явной фразой «`docs/CANVAS.md` не + обновляется ради hover ownership». +- **AC1…AC16.** Каждый критерий сформулирован как проверяемое числовое или + структурное утверждение (допуски `±0.005`, `±20 ms`, `≤0.5 CSS px`, + `44×44 CSS px`) и несёт способ доказательства (`unit`/`smoke`/`golden`/ + `code review`/`performance profile`), как того требует DoR (PROCESS.md + §2.5). Ни один AC не сформулирован через реализацию так, что критерий + нельзя было бы фальсифицировать независимо от кода. +- **Данные/миграция/i18n.** §15 корректно фиксирует отсутствие новых + config/localStorage/backend-полей и отсутствие миграции; совместимость с + `docs/CONFIG-COMPATIBILITY.md` не требует новой записи, так как round-trip + не меняется — проверено чтением раздела «Status meanings» в этом документе. +- **Touch-контракт.** Трейлер `Touch editor: best effort / intentionally + degraded` относится к редакторам (Device/Plan/Background остаются + desktop-first, §14), тогда как View/kiosk-touch — обязательная, + release-blocking часть скоупа (AC8–AC12) в полном соответствии с + `docs/TOUCH-SUPPORT.md` («A touch-only failure in View is a product + defect»). Формат трейлера соответствует одному из трёх канонических + значений документа. +- **Откат.** §20 указывает на одиночный revert реализации без миграции + данных и без storage rollback — согласуется с «Migration не нужна» (§15). +- **Никаких искусственно вычисленных «Fresh»-цифр.** Числа 0.90/0.95/200 ms + трассируются либо к телу #212 (5%, 0.2s), либо к уже принятому ТЗ #22 + (перенос контракта eligibility/confirmation/reduced-motion, §4), а не + придуманы автором ТЗ. +- **Единственный открытый вопрос не требуется.** Явных продуктовых развилок, + которые нельзя было бы разрешить чтением #212/#22/#154/#181 и уже принятых + #179/#211, не найдено — комментарий аналитики владельца прямо закрывает + этот путь («Продуктовых вопросов не осталось»), а единственная найденная + двусмысленность (Low-1) не поднимается до уровня, требующего его участия. + +## Чего не проверял + +- Не запускал `npx tsc --noEmit`, `npm test`, `npm run build` — на этапе + `spec` кода ещё нет, эти гейты относятся к код-ревью (PROCESS.md §2.7) и + реализации (§2.6). +- Не запускал browser-смоки/golden — они тоже принадлежат циклу реализации + и код-ревью, а не ревью ТЗ. +- Не проверял `custom_components/**/*.py` — ТЗ прямо заявляет отсутствие + изменений бэкенда и новых config-полей (§15), backend-путей в + «Ожидаемые файлы» (§16) нет. +- Не оценивал реализуемость 200 ms/0.95 анимации в конкретном браузерном + движке (WAAPI vs CSS transition) — это явно помечено в §22 как «принято + предположительно, ревьюер вправе изменить свободно» и будет предметом + код-ревью, а не ревью ТЗ. + +## Итог + +`docs/specs/212-device-icons-polish.md` — самодостаточное, добросовестно +проверяемое ТЗ: каждое фактическое утверждение о текущем коде подтверждено +построчной сверкой, каждый AC проверяем и несёт способ доказательства, скоуп и +не-скоуп аккуратно очерчены и не задевают lock-инвариант или другие +ограничители `docs/SCOPE.md`, канонический адрес документации (#181) +исправлен верно. Единственная находка — Low, снимается с запиской автору, не +блокирует переход в «Готово к разработке». + +**Вердикт: зелёный · цикл r1/4 · High: 0 · Medium: 0 в задаче**