From 4b46a6250344f67b2d1a38a6de38b0655cb6f22d Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Tue, 18 Aug 2026 19:31:44 +0000 Subject: [PATCH] docs: review document for #154 Issue: #154 User-Visible: no --- docs/reviews/SPEC-REVIEW-154-r1.md | 295 +++++++++++++++++++++++++++++ 1 file changed, 295 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-154-r1.md diff --git a/docs/reviews/SPEC-REVIEW-154-r1.md b/docs/reviews/SPEC-REVIEW-154-r1.md new file mode 100644 index 00000000..e6307410 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-154-r1.md @@ -0,0 +1,295 @@ +# Ревью ТЗ — issue #154, цикл r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/154 +- **ТЗ:** `docs/specs/154-touch-hover-reset.md`, коммит `cd3c7752a8846c11508a7a6b4c46b6d990624408` +- **Этап:** spec (PROCESS.md §2.4) +- **Вердикт:** жёлтый · цикл r1/4 · High: 3 · Medium: 1 + +## Скоуп ревью + +Прочитаны в указанном порядке: `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` +(§1–§4, §7, §12), тело issue #154 и оба комментария владельца (аналитика +2026-08-15: P1/bug/polish, обычный трек, «trivial запрещён из-за touch +impact», «вопросов нет»; публикация ТЗ), `docs/USER-GUIDE.ru.md` (термины +«hover», раздел ограничений «Комнатные подсказки основаны на hover»), +`docs/TOUCH-SUPPORT.md` (input classification, deliberate degradation), +`docs/UX-MODES.md` (contract View: «room hover highlight, hover tooltips»), +`docs/CANVAS.md` (проверка, действительно ли это канонический документ для +hover-контракта — нет, см. Medium-1) и `docs/TESTING.md` (существующий +контракт «No hover tooltips on touch», v1.43.3/v1.42.2). + +Issue не помечен `small`/`trivial`; ТЗ корректно лежит файлом в +`docs/specs/154-touch-hover-reset.md`, а не в теле issue. Формат ревью — +полный документ, не комментарий. + +Для калибровки того, что этот репозиторий уже считает обязательным +оформлением §7.1, сверился с `docs/specs/138-adjacent-room-autoclose.md` +(разделы «1. Сценарий и продуктовый контекст» / «2. Что человек увидит до и +после») и его ревью `docs/reviews/SPEC-REVIEW-138-r1.md` — оно отдельной +строкой подтверждает наличие обоих разделов как условие «обязательные +разделы §7.1 на месте». + +## Как проверялось + +Ревью также читало код, на который ТЗ опирается как на текущее поведение — +чтобы отличить обоснованное утверждение от догадки, выданной за факт: + +- `src/houseplan-card.ts:5442–5470` (`_notePointer`) — подтверждает + формулировку issue и ТЗ: touch/pen действительно закрывает `_tip` + (`5446`), но не трогает `_hoverRoom`. Совпадает. +- `src/houseplan-card.ts:659,662,1759–1760` — `_tip`/`_hoverRoom` как + реактивный state; `14619–14646` — room hover действительно навешан через + `@mouseenter`/`@mouseleave` на path/polygon/rect комнаты, без + `pointerenter`/`pointerleave`. Совпадает с §6 ТЗ. +- `src/houseplan-card.ts:1285,5443,9494,11167` — уже существует + `_boundaryPointerType`, session-local поле «последний реальный + pointerType», используемое для decor-snap радиуса (`_decorSnap`) и + touch-детекции (`9494`). ТЗ §5 говорит «Houseplan вводит один + session-local источник фактической modality», не упоминая, что подобное + поле уже есть. Это не продуктовая двусмысленность (реализация — дело + автора, PROCESS §7.1), но реальный риск дублирования authority; отмечено + ниже как **Low**, без отдельного issue. +- `grep ':hover'` по `src/` — 32 вхождения в 6 файлах + (`houseplan-card.ts`, `styles.ts`, `space-card.ts`, `hp-dialog.ts`, + `hp-device-preview.ts`, `hp-color-opacity.ts`, `hp-help.ts`). Ни один не + назван в ТЗ — см. **High-3**. +- `docs/CANVAS.md` — единственное вхождение слова «hover» (строка 563, + «hover is only a preview and never authoritative») относится к preview + hit-резолвера архитектурного connection overlay в редакторе Плана + (endpoint/line snapping), а не к View-контракту room/device hover, + который меняет issue #154. `docs/UX-MODES.md:69–70` прямо перечисляет + «room hover highlight, hover tooltips» как часть допустимых + взаимодействий View — см. **Medium-1**. +- `docs/TESTING.md:175–176,750–752` — существующий, уже протестированный + контракт «a hover tooltip never appears after ANY touch/pen pointer + event, even if the browser claims hover: hover» — ТЗ корректно описывает + его как частично реализованный и подлежащий расширению. +- issue #152 (`gh issue view 152`) — действительно ещё `S4-spec-review`, не + реализован; ссылка ТЗ на «будущий #152» и решение не читать `_hoverRoom` + из его hit-резолвера корректны и не создают ложной зависимости. + +## Находки + +### High-1 — нет обязательных разделов «Сценарий» и «Что человек увидит до/после» (§7.1) + +**Где:** `docs/specs/154-touch-hover-reset.md`, весь документ; ближе всего +раздел «1. Проблема и требуемый результат» (строки 9–19). + +PROCESS.md §7.1 требует ТЗ начинать с двух продуктовых разделов: «какая +персона (`docs/SCOPE.md`), на какой поверхности, в какой момент это +встретит» и «что человек увидит до и после, одной фразой без терминов +реализации» — и подчёркивает, что «два первых раздела — продуктовые, и они +идут первыми не случайно». Это не стилистическая рекомендация: сам +репозиторий уже применяет это буквально в `docs/specs/138-...md` +(разделы «1. Сценарий и продуктовый контекст» / «2. Что человек увидит до и +после»), и предыдущее ревью того ТЗ отдельно проверяло их наличие как +условие «обязательные разделы §7.1 на месте». + +В ТЗ #154 такого раздела нет вообще. Раздел 1 сразу входит в техническое +описание бага (`_notePointer()`, CSS `:hover`, `mouseenter`/`mouseleave`), а +второй абзац, который мог бы быть «что человек увидит», написан в терминах +реализации: «показывает только краткий pressed feedback», «настоящие mouse и +trackpad», «на гибридном устройстве последующий mouse input восстанавливает +его» — это не «одна фраза без терминов реализации», а пересказ технического +контракта. + +Персона и поверхность из `docs/SCOPE.md` — не абстракция: J1–J3 явно относят +View к household members/guests на wall tablet и companion-приложении +(«View mode is the product for two of the three personas»), а issue +явно называет ещё и «мобильный браузер / HA Companion». Отсутствие +явного раздела означает, что читатель ТЗ вынужден реконструировать это +сам, а вопрос «какая персона важнее при конфликте» (например, «жертвуем ли +чем-то ради гибридного admin-устройства ради гарантии touch у household +member») в тексте не решён явно — только подразумевается порядком risk-строк +§16. + +**Почему High:** это единственная площадка процесса, где владелец мог бы +опротестовать выбор персоны/поверхности/степени деградации ДО написания +кода (PROCESS §7.1: «Владелец отвечает на продуктовые вопросы… их место — +этап ТЗ»); без явного раздела с этим согласиться (или оспорить) нечего — +ревьюер не может подтвердить, что «что человек увидит» действительно решено, +а не растворено в технической формулировке. + +**Что нужно поправить:** добавить два раздела по образцу #138 — «Сценарий» +(персона/поверхность/момент, со ссылкой на J1–J3 `docs/SCOPE.md`) и «Что +человек увидит до/после» одной фразой без «modality», «pointerType», +«hybrid», «pressed feedback» (например: «раньше подсветка комнаты/маркера +после тапа могла остаться до следующего действия; теперь она гаснет сразу +после отпускания пальца, а мышь на компьютере работает как раньше»). + +### High-2 — критерии приёмки не указывают способ доказательства (DoR §2.5) + +**Где:** `docs/specs/154-touch-hover-reset.md` §12 «Acceptance criteria» +(строки 197–212), сверено с §13 «План тестирования» (214–252). + +PROCESS.md §2.5 (вход в «Готово к разработке») требует буквально: «AC1…ACn — +пронумерованные проверяемые критерии приёмки; **у каждого указано, чем он +доказывается**: `unit` / `backend` / `smoke` / `golden` / «ревью кода»» — и +«если хоть один пункт не выполнен — статус не «Готово к разработке»». + +Все 12 пунктов §12 сформулированы как утверждения о поведении (хорошо, +проверяемо и однозначно сформулированы — отдельная претензия к ним +отсутствует), но ни один не помечен способом доказательства. §13 группирует +тесты по категориям (Unit/source contract, Browser smoke, Golden, +Performance), но не сопоставляет их с номерами AC — например, неясно из +текста, доказывается ли AC7 («desktop mouse hover не изменён») smoke-тестом, +golden-скриншотом, или обоими одновременно, и то же для AC9 (keyboard focus) +и AC12 (naked hover selector — очевидно «ревью кода» по source-inventory, +но это нигде не написано явно). + +**Почему High:** это не редакторская придирка, а буквальный пункт входного +чек-листа перед `S5-ready`. Без явной привязки разработчик и код-ревьюер +(другая модель, без контекста этой сессии — AGENTS.md «Two-agent workflow») +должны будут заново реконструировать, каким тестом каждый AC закрыт, что +именно и должен исключать этот пункт DoR. + +**Что нужно поправить:** добавить к каждому из 12 пунктов §12 тег +доказательства, например `[unit]`, `[smoke]`, `[golden]`, `[unit+smoke]`, +`[ревью кода]` — материал для этого уже есть в §13, требуется только явное +сопоставление 1:1. + +### High-3 — не перечислены затронутые файлы и модули (DoR §2.5) + +**Где:** весь документ; ближайший к теме раздел — «14. План реализации» +(строки 254–262). + +Тот же пункт DoR §2.5 требует отдельно: «перечислены затронутые файлы и +модули». В ТЗ этого списка нет — §14 ограничивается общими шагами +(«провести inventory JS state и View/shared CSS hover selectors», «ввести +component-local pointer modality authority»), не называя ни одного +конкретного файла. + +Это не тривиальный однофайловый фикс: `grep ':hover' src/` даёт 32 +вхождения в 6 файлах — `houseplan-card.ts`, `styles.ts`, `space-card.ts`, +`hp-dialog.ts`, `hp-device-preview.ts`, `hp-color-opacity.ts` (плюс +`hp-help.ts`, у которого есть top-level `mouseenter`/`_notePointer`-подобная +логика — исключён по контракту §4, но это тоже стоило бы явно назвать). +Раз §3 ТЗ прямо говорит, что «общие компоненты редакторов получают +исправление, если используют тот же pointer-modality gate», а этот gate +должен быть «единым» и «передан в обязательные shadow child components» +(§5), явно неясно без списка файлов, какие из этих 6+ компонентов входят в +обязательный охват, а какие — только в «если не требуют отдельной сложной +адаптации» (§3, необязательная часть). + +**Почему High:** без списка файлов нельзя проверить полноту заявленного +«обязательного охвата» View (issue, раздел «Обязательный охват») — то же +самое, что нельзя проверить AC без способа доказательства. + +**Что нужно поправить:** добавить раздел «Затронутые файлы» с явным списком +(минимум все 6 файлов с `:hover`, плюс модуль(и), где будет жить pointer +modality authority), и отметить для каждого — «обязательный охват» или +«если дёшево, без специальной адаптации» (§3). + +### Medium-1 — план документации (§15) называет неверный канонический документ для hover-контракта + +**Где:** `docs/specs/154-touch-hover-reset.md` §15 (строки 264–276), пункт +«`docs/CANVAS.md` — hover/pressed/semantic ownership». + +`docs/CANVAS.md` — канонический документ infinite canvas (координаты, +content frame, zoom/pan, grid, drag limits, snap contract, wall geometry). +Слово «hover» встречается там ровно один раз (строка 563) и относится к +preview hit-резолверу architectural connection overlay в редакторе Плана +(«the same resolver runs again on click, so hover is only a preview and +never authoritative») — это про предпросмотр примыкания к стене при +рисовании, а не про View-контракт room/device/opening hover, который меняет +эта задача. + +Настоящий канонический источник контракта — `docs/UX-MODES.md`, раздел +«View — display and device interaction only» (строки 63–70): «room hover +highlight, hover tooltips (name, clean-floor area, temperature, signal)» — +именно эта строка перечисляет ровно то поведение, которое ТЗ #154 меняет +(добавляет pointer-modality gate и запрещает sticky/synthetic hover). ТЗ +§15 не включает `docs/UX-MODES.md` в список документов на обновление +вообще. + +**Последствие, если не исправить:** implementer, следуя §15 буквально, +допишет в `docs/CANVAS.md` абзац не по адресу (документ и так не про +интеракции), а `docs/UX-MODES.md` продолжит говорить просто «room hover +highlight, hover tooltips» без указания на pointer-modality/no-sticky-hover +контракт — расхождение документации с реальным поведением ровно там, где +его будут искать в первую очередь (`UX-MODES.md` явно значится и в +AGENTS.md как канонический документ подсистемы). + +**Что нужно поправить:** заменить `docs/CANVAS.md` на `docs/UX-MODES.md` в +списке §15 (либо добавить `UX-MODES.md` к списку и явно решить, нужен ли +`CANVAS.md` вообще — по факту нет, там нет ownership hover). + +Заведён отдельный issue: см. итог ниже. + +## Что проверено и корректно + +- **Проблема описана по реальному коду, не по догадке.** `_notePointer()` + (`houseplan-card.ts:5442`) действительно закрывает только `_tip`, room + hover действительно висит на `mouseenter`/`mouseleave` + (`14619–14646`) — оба факта, на которых строится вся мотивация ТЗ, + подтверждены чтением, не голословны. +- **Границы transient hover (§2) чёткие и без скрытого расширения скоупа.** + Working/alarm/unavailable/Glow/pulse, `hp-help`, selected/checked, + keyboard focus прямо исключены из очистки — совпадает с исключениями, + которые перечислил сам владелец в теле issue («Исключения»). +- **Synthetic-mouse policy (§6) корректно закрывает главный технический + риск бага** («мобильный браузер может синтезировать mouse-события») — + явно требует trusted `PointerEvent` с реальным `pointerType`, запрещает + произвольный wall-clock timeout как основной механизм. Это прямое, + проверяемое техническое решение, а не «наверное сработает». +- **Не создаёт ложной зависимости от нереализованного #152.** Проверено: + #152 (`click-to-fit`) сам ещё в `S4-spec-review`; §9 ТЗ прямо пишет, что + #152 должен использовать canonical hit resolver независимо от + `_hoverRoom`, не читая его как заменитель выбора — корректно, не + предвосхищает нерешённый ТЗ #152. +- **«Принятые предположения» (§17) — действительно технические или уже + решённые владельцем, не спрятанные продуктовые вопросы.** Пункт «touch и + pen следуют одинаковой no-hover policy» дословно совпадает с «Pen tap + следует touch-политике» из тела issue (владелец уже решил это), а не + является домыслом автора ТЗ; «media query — только второй gate», + «mouse modality — только trusted PointerEvent» — реализационные решения, + которые PROCESS §7.1 явно отдаёт на усмотрение автора. +- **Accessibility (§11) не оставляет открытых вопросов**: DOM focus, + `:focus-visible`, `aria-*`, dialog focus trap явно выведены из-под + touch-cleanup — совпадает с «Keyboard focus и `:focus-visible` сохраняют + штатное поведение» из тела issue. +- **i18n и модель данных закрыты корректно и коротко, а не отпиской.** §15 + прямо говорит: новых строк не ожидается, если появятся — обе локали и + parity test обязательны; §16 — «Config, storage и backend data не + меняются; миграция и data rollback не нужны» — соответствует тому, что + задача чисто presentation/pointer-lifecycle, backend не тронут. +- **Откат (§16) реалистичен**: «возвращает прежние JS/CSS hover paths» — + для чисто клиентской, не мигрирующей данные задачи это адекватный и + проверяемый уровень детализации отката. +- **Трек и трейлеры соответствуют аналитике.** Комментарий 2026-08-15 + корректно исключил `trivial` (touch impact) и `small` (несколько + поверхностей: room/device/opening/controls/dialogs); полный трек и файл + в `docs/specs/` — правильный выбор, подтверждён `S4-spec-review` без + `small`. + +## Чего не проверял + +- Реализацию — её нет, это ревью ТЗ, а не кода (PROCESS.md §2.4). +- Golden/browser smoke живьём — на этапе ТЗ не запускаются; план в §13 + выглядит технически осмысленным (real touch context, CDP touch input, + `(any-hover: hover)` контекст), но существование конкретных сценариев не + верифицировалось запуском. +- Производительность — утверждения §13 «Performance» (pointermove не + вызывает full Lit update и т.п.) не профилировались; на этапе ТЗ это не + требуется, оценка отложена до реализации/пре-релизного гейта. +- Полный список всех `:hover`-селекторов в `styles.ts` построчно (нужен ли + каждому modality gate) — за пределами ревью ТЗ; сосчитано только число + файлов/вхождений, чтобы обосновать High-3. +- Реализуемость unit-теста «compatibility MouseEvent не включает mouse + modality» в текущем test harness (`test/**`) — не проверялась: на этапе + спецификации это заявление о контракте, а не код. + +## Вывод + +Технический контракт (§5–11, §16–17) продуман, обоснован кодом и не +содержит выданных за факт догадок — это сильная сторона документа. Но три +independent High-находки — отсутствие обязательных продуктовых разделов +§7.1, отсутствие способа доказательства у каждого AC и отсутствие списка +затронутых файлов — это три отдельных, буквальных пункта DoR §2.5, +неотвеченных полностью. Ни один из них не требует пересмотра решения, все +три чинятся добавлением текста в это же ТЗ без изменения контракта. + +Возврат в «ТЗ в работе» (`S3-spec`), цикл r1/4. + +Medium-1 заведён отдельным issue со ссылкой на #154 (не блокирует переход +сам по себе, но должен быть закрыт до релиза документации).