From 68992d558258ba7d1ea2f6bda766127e2b9b716e Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Tue, 18 Aug 2026 19:36:36 +0000 Subject: [PATCH] docs: spec review document for #152 Issue: #152 User-Visible: no --- docs/reviews/SPEC-REVIEW-152-r1.md | 285 +++++++++++++++++++++++++++++ 1 file changed, 285 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-152-r1.md diff --git a/docs/reviews/SPEC-REVIEW-152-r1.md b/docs/reviews/SPEC-REVIEW-152-r1.md new file mode 100644 index 00000000..8c0878be --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-152-r1.md @@ -0,0 +1,285 @@ +# Ревью ТЗ — issue #152, цикл r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/152 +- **ТЗ:** `docs/specs/152-room-click-fit.md`, ветка `issue/152-room-click-fit`, + коммит `7f8751b4dce84880edaab95600be7f028b9fab5f` +- **Этап:** spec (PROCESS.md §2.4) +- **Вердикт:** красный · цикл r1/4 · High: 2 · Medium: 2 + +## Скоуп ревью + +Прочитаны в указанном порядке: `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (§1, +§2, §3, §4, §7, §8, §12), тело issue #152 и все три комментария (уточнение и +самоотзыв про double-tap на фоне, актуализация аналитики 2026-08-15, хендофф +автора ТЗ), `docs/USER-GUIDE.ru.md` (терминология «Карточка комнаты», клик по +комнате в Resize, пространство/комната), `docs/UX-MODES.md`, `docs/TOUCH-SUPPORT.md`, +`docs/CANVAS.md` (§4–8: content frame, ZOOM_MIN/MAX, viewport/editor handoff), +`docs/CONFIG-COMPATIBILITY.md`. Issue не помечен `small`/`trivial` (аналитика: +сложность 4/10, риск 5/10, «обычный трек»), поэтому ТЗ корректно вынесено в +отдельный файл `docs/specs/152-room-click-fit.md`, а не в тело issue. Формат +ревью — полный документ. + +## Как проверялось + +ТЗ активно ссылается на «существующее» поведение как на данность (существующий +resolver, существующий double-tap, существующая доступная подпись, существующие +ZOOM_MIN/MAX). Каждое такое утверждение сверено с реальным кодом, а не принято +на веру: + +- `src/houseplan-card.ts:4792-4793`, `src/space-geometry.ts:203` — `ZOOM_MIN`/ + `ZOOM_MAX`/`MIN_ZOOM` существуют, значения `8` и `1/3` подтверждены. +- `src/houseplan-card.ts:5023-5035` (`_resetZoom`) и `:5264-5300` + (`_stagePointerUp`) — double-tap-to-reset реализован, но целиком внутри + `if (this._kiosk) { … }`; вне этого блока на stage нет ни `@dblclick`, ни + иной обработки двойного тапа (проверено также по полному списку обработчиков + стейджа, `:14471-14480`). +- `src/houseplan-card.ts:14619-14646` (room hover через нативные + `@mouseenter`/`@mouseleave` на каждой SVG-форме комнаты) против + `:6528, :9936, :11045, :11096` (`[...rooms].reverse().find(r => this._pointInRoom(raw, r))`, + используется инструментами Resize/Delete/Merge/Split) — это два разных + механизма, не один вызываемый resolver. +- `src/houseplan-card.ts:16001-16065` (`_renderRoomLabel`) — `.roomlabel` не + имеет `tabindex`/`role`/`aria-label`/`@keydown`; единственные `tabindex` в + файле — `tabindex="-1"` на editor-тулбарах (`:9241, :16963, :17044`), + подтверждает `docs/SCOPE.md` («no ARIA labelling of the plan»). +- `gh issue view 82` — открыт, всё ещё `S3-spec`: гипотеза «до реализации #82» + в ТЗ корректна, `ViewportAnimator` в `src/` действительно не найден (grep). +- `gh api repos/Matysh/houseplan-card/issues/28` — `state: closed, + state_reason: not_planned`, закрыт 2026-08-14, то есть до публикации этого ТЗ. +- `docs/specs/146-four-phase-sun-background.md` §15 и + `docs/specs/138-adjacent-room-autoclose.md` §13 — эталон формата AC с + инлайн-аннотацией способа доказательства, использован как мера сравнения. + +## Находки + +### High-1 — ни один AC не указывает способ доказательства + +**Где:** `docs/specs/152-room-click-fit.md` §11 (строки 172–187). + +Раздел перечисляет 14 пронумерованных критериев без пометки `unit`/`backend`/ +`smoke`/`golden`/«ревью кода» у каждого — в отличие от §12 «План тестирования», +который группирует проверки по категории, но не привязывает их к номерам AC. +Например, AC5 («Flat и Isometric проверяются по итоговой экранной геометрии») +может быть закрыт unit-тестом проекции, browser smoke со скриншотом или +golden-эталоном — все три упомянуты в §12, и по тексту ТЗ невозможно понять, +какой из них действительно обязателен для приёмки, а какой — просто +сопутствующий. + +PROCESS.md §2.5 (DoR) требует буквально: «AC1…ACn — пронумерованные проверяемые +критерии приёмки; у каждого указано, чем он доказывается: unit / backend / +smoke / golden / «ревью кода»». Соседние ТЗ этого же репозитория соблюдают это +дословно (`docs/specs/146-four-phase-sun-background.md:417-465`: «**AC1 +(`unit`; разработчик):**» и так для всех 16 пунктов; аналогично +`docs/specs/138-adjacent-room-autoclose.md:276-323`). ТЗ #152 — единственное +из свежей серии, где это правило не выполнено ни разу. + +**Почему High, а не Low:** это не стилистическая придирка, а прямое условие +готовности к разработке (§2.5) и то, что мне прямо поручено проверить в этом +ревью («указание способа доказательства» для каждого AC). Без этой аннотации +issue не может законно перейти в `S5-ready`. + +**Что нужно поправить:** проставить у каждого из AC1–AC14 способ доказательства +и исполнителя, как в §15 `docs/specs/146-...md`. + +### High-2 — потерян отдельный проверяемый AC про синхронность hit targets во время перехода + +**Где:** `docs/specs/152-room-click-fit.md` §8 (строки 130–151) и §11 (172–187); +ср. issue #152, пункт критериев приёмки «Viewport и все SVG/HTML hit targets +остаются синхронны во время перехода» (14-й из 15 чекбоксов тела issue). + +Это требование в финальном ТЗ не стало отдельным AC — оно осталось прозой +внутри §8: «Viewport, SVG и HTML overlays получают один и тот же camera state +на каждом кадре; промежуточное рассогласование hit targets недопустимо.» Ни +одного номера в §11, ни отдельного пункта browser smoke в §12 (там есть +«Flat/Isometric screenshot bounds; ни один видимый слой или hit target не +отстаёт» — но это один общий пункт про финальный кадр, а не про сам переход +кадр за кадром) под это не выделено. + +**Сценарий, которому это открывает дорогу:** как только заработает `ViewportAnimator` +(#82) и переход из клика по комнате в её fit станет анимированным tween'ом, +визуальный viewBox обновляется на каждом кадре, а SVG/HTML hit-элементы +(устройства, проёмы, room label) должны обновляться синхронно. Если реализация +пересчитывает hit-геометрию на кадр позже видимой, пользователь тапает туда, +где устройство видно **сейчас**, и промахивается — либо получает клик по +случайно оказавшемуся там другому объекту. Именно этот класс дефекта +(«hit target отстаёт от кадра во время перехода») уже упомянут как прецедент в +`AGENTS.md` (#89 — «смок, который не умел падать» стоил целого цикла ревью), и +без отдельного AC с проверяемым доказательством ничто не гарантирует, что тест +на это вообще напишут, а не просто положатся на прозу §8. + +**Почему High:** это не новая функциональность сверх скоупа — она уже была +явно сформулирована владельцем в теле issue как критерий приёмки и молча +выпала при переносе в ТЗ. Восстановление контракта — не расширение задачи, а +исправление ТЗ до его полноты. + +**Что нужно поправить:** вернуть требование как отдельный пронумерованный AC с +указанным способом доказательства (browser smoke с промежуточными кадрами +tween, не только конечным) и явной строкой в §12. + +### Medium-1 — accessibility-раздел выдаёт «существующую accessible-подпись» за факт, хотя такой доступности сегодня нет + +**Где:** `docs/specs/152-room-click-fit.md` §9 (строки 153–163), формулировка +«Существующая доступная подпись либо room-card target получает action Enter и +Space». + +`.roomlabel` (`src/houseplan-card.ts:16043-16056`, `_renderRoomLabel`) сегодня +не имеет ни `tabindex`, ни `role`, ни `aria-label`, ни `@keydown` — только +pointer-обработчики для drag в Plan-редакторе и клик на дочерней `ha-icon` +(переход в HA area). Единственные `tabindex` во всём файле — три +`tabindex="-1"` на editor-тулбарах (`:9241, :16963, :17044`), что снимает tab- +stop, а не добавляет. Это согласуется с прямой формулировкой `docs/SCOPE.md` +(«Accessibility: … no ARIA labelling of the plan») — сегодня в плане нет ни +одного клавиатурно-доступного или ARIA-размеченного элемента вообще. + +Формулировка §9 читается как «добавить действие к уже доступному элементу», а +по факту задача вводит **первый** в продукте клавиатурно-фокусируемый, +ARIA-размеченный элемент плана, причём по одному на каждую комнату — со всеми +последствиями (порядок табуляции по потенциально многим комнатам, +обнаруживаемость, стили `:focus-visible`, отсутствие конфликта с уже занятыми +`tabindex="-1"` зонами тулбаров). Раздел §9 сам по себе технически осторожен +(явно запрещает невидимую полноэкранную сетку tab-stop), но опирающееся на него +предложение 1 и допущение из AC12 создают впечатление меньшего объёма работы, +чем есть на самом деле, и не фиксируют явно, что это новая a11y-поверхность, а +не расширение существующей. + +**Почему Medium:** это не блокирует ни один AC — предложенное поведение +реализуемо, задача была явно запрошена в теле issue (пункт про Enter/Space уже +был в исходных критериях). Но формулировка занижает объём и скрывает +продуктовый факт (первая ARIA-разметка плана), который стоит зафиксировать +явно, а не как побочный эффект неточной фразы. + +**Заведён отдельный issue** https://github.com/Matysh/houseplan-card/issues/182. + +### Medium-2 — «существующий» double-click/tap fit-all на фоне на деле существует только в kiosk + +**Где:** `docs/specs/152-room-click-fit.md` §3 («не входит… изменение +существующего double-click/tap fit всего плана на свободном фоне»), §7 +(«Double click/tap по свободному фону сохраняет существующий fit/reset всего +плана без изменений»). + +`docs/UX-MODES.md` документирует double-tap-reset только в разделе «Kiosk mode +(v1.41.0)»: «double tap resets zoom». В разделе «View» такого жеста нет вовсе. +Чтение кода подтверждает документацию: логика `_lastTap`/`_resetZoom()` в +`_stagePointerUp` (`src/houseplan-card.ts:5264-5277`) целиком находится внутри +`if (this._kiosk) { … }`; на самом stage нет `@dblclick` (полный список +обработчиков — `:14474-14480`). То есть на обычном (не kiosk) View — ни +десктопный dblclick мышью, ни touch double-tap — сегодня **ничего** не делают +на фоне; жест существует только в kiosk. + +ТЗ формулирует это как факт про View в целом, а не как допущение или явно +ограниченное kiosk-наблюдение (в отличие от §16, где рядом стоящее допущение +«до #82 переход атомарный» правильно вынесено в блок предположений). Первый +комментарий владельца к issue («двойной клик/тап… – Поправка, это поведение +кажется уже реализовано») показывает, что сама неоднозначность уже была на +поверхности при заведении issue и не была закрыта явной проверкой. + +**Последствия:** (1) пункт browser smoke «double tap свободного фона сохраняет +прежнее поведение» (§12) содержателен только для kiosk — для обычного View +«прежнее поведение» это просто отсутствие эффекта, что тривиально пройдёт даже +при ошибке в общей gesture-арбитраже; (2) для ownership-логики §6–7 (отличить +одиночный tap от первой половины double-tap) потребуется **новая** общая +(не только kiosk) логика распознавания double-tap — её сегодня не существует, +а формулировка «новые независимые константы не вводятся» (§7) намекает, что +общий тайминг уже есть и его достаточно переиспользовать, что неверно вне +kiosk. + +**Почему Medium:** не блокирует ни один AC (инструкция «сохранить как есть» +остаётся корректной независимо от истинного текущего охвата), но фактическая +неточность способна привести к смоку, проверяющему не то окружение, и к +недооценке объёма новой gesture-арбитражной логики. + +**Заведён отдельный issue** https://github.com/Matysh/houseplan-card/issues/183. + +## Заведённые issue по Medium-находкам + +- **Medium-1** → https://github.com/Matysh/houseplan-card/issues/182 +- **Medium-2** → https://github.com/Matysh/houseplan-card/issues/183 + +## Что проверено и корректно + +- **Геометрический контракт (§4)** — однозначен и самосогласован: safe rect + `0.8W×0.8H`, формула `scale = min(0.8W/Rw, 0.8H/Rh)`, пример 2000×1000 → + 800×800 с полями 100/100/600/600 px — арифметика проверена вручную и + совпадает с телом issue дословно. +- **`ZOOM_MIN`/`ZOOM_MAX` действительно существуют** (`houseplan-card.ts:4792-4793`, + `space-geometry.ts:203`, значения 8 и 1/3) — ссылка на «существующие пределы + zoom» не выдумана. +- **#82 корректно учтён как ещё не реализованный** — issue #82 открыт, + `S3-spec`; `ViewportAnimator` в `src/` не найден (grep). Условная + формулировка «до реализации #82 … атомарно» точна, а не догадка. +- **Конфликт с #28 (§10)** — сам факт конфликта (обычный клик не может + одновременно принадлежать двум действиям) реален, а решение (primary click + закреплён за fit-to-room) — явное продуктовое решение, а не техническая + подмена; см. также Low-1 про устаревший статус #28 ниже. +- **Модель данных и миграция** — по существу закрыто корректно: room fit не + меняет `room`/`space`/backend-схему, новых полей не появляется; сверено с + `docs/CONFIG-COMPATIBILITY.md` (нет ни одной точки пересечения с текущим + реестром совместимости). Разнесено по §8/§15 вместо отдельного раздела + (см. Low-3), но по содержанию пробелов нет. +- **Non-scope (§3)** согласован с non-scope тела issue и не оставляет скрытого + расширения (открытие карточки, кнопка «назад», авто-смена пространства, + свободное вращение изометрии, второй animator — всё явно исключено). +- **Блок предположений (§16)** используется по назначению: пункты про базис + измерения 10%, состав bounds, атомарность до #82 и передачу trigger карточки + в #28 — это действительно технические/уточняющие решения, а не спрятанные + продуктовые вопросы, которые следовало адресовать владельцу. Ревьюер + оспаривает формулировку пункта про «единственный resolver» (см. Low-4) в + рамках штатного технического спора автор/ревьюер, не как продуктовый вопрос. +- **i18n** — конкретный текст на двух локалях дан («Вписать комнату Гостиная» / + «Fit room Living room»); имена ключей не выбраны, но это техническая деталь + именования, а не продуктовая неясность. + +## Low (снято с записью, не блокирует) + +- **Low-1.** §10 обсуждает #28 как открытую задачу, которой перед реализацией + «нужно изменить UX». По факту #28 закрыт `not_planned` 2026-08-14 — до + публикации этого ТЗ. Само продуктовое решение (primary click → fit-to-room) + от этого не меняется, но формулировку стоит поправить в следующей редакции, + чтобы не выглядела как отсылка к живой задаче. +- **Low-2.** §1 не называет явно персону из таблицы `docs/SCOPE.md` (Home admin / + Household members / Guests) — сценарий описан через поверхность (View, + kiosk) и типичный триггер (большой/сложный план), персона угадывается, но не + названа. Не мешает пониманию контракта. +- **Low-3.** В отличие от `docs/specs/146-...md`/`138-...md`, в ТЗ #152 нет + отдельного заголовка «Модель данных и миграция» — содержание корректно, но + разнесено по §8 и §15. Рекомендация на будущее — для единообразия со + смежными ТЗ этого репозитория. +- **Low-4.** §16 полагается на «единственный existing room hover/hit resolver». + По коду это два разных механизма: hover в View — нативные + `@mouseenter`/`@mouseleave` на SVG-форме каждой комнаты (`:14619-14646`), а + инструменты Resize/Delete/Merge/Split используют собственный + `[...rooms].reverse().find(pointInRoom)`, продублированный в 4 местах + (`:6528, :9936, :11045, :11096`). Оба, вероятно, совпадают на практике + (одинаковый z-порядок), но структурно это не один переиспользуемый + resolver. Пункт уже явно оформлен как допущение в §16, поэтому не требует + отдельного issue — фиксирую как технический спор для реализации: имеет + смысл вынести один явный shared-хелпер, а не полагаться на совпадение двух + независимых путей. + +## Чего не проверял + +- Реализацию — её нет, это ревью ТЗ, а не кода. +- Golden/визуальные артефакты и производительность — на этапе ТЗ не + требуются; §12 корректно откладывает их проверку до реализации/пре-релиза. +- Backend/`custom_components/houseplan/**/*.py` — задача их не касается по + заявленному scope, отдельно построчно не сверялось, так как заявлено + «room fit не меняет model/backend», а `docs/CONFIG-COMPATIBILITY.md` не + показал точек пересечения. +- Полное соответствие проектируемого «одного focus target на комнату» + практикам ARIA authoring patterns — это вопрос code review на этапе + реализации, не ТЗ. +- Не выполнялись гейты (`typecheck`/`test`/`build`) — на этом этапе нет кода + для их запуска, только документ. + +## Вывод + +Два блокирующих дефекта: (1) ни один из 14 AC не указывает способ +доказательства, что напрямую противоречит DoR (PROCESS.md §2.5) и практике +соседних ТЗ этого репозитория; (2) явный критерий приёмки issue про +синхронность SVG/HTML hit targets с viewport во время перехода потерялся при +переносе в ТЗ и не имеет ни номера AC, ни отдельной smoke-проверки. Оба +дефекта устраняются правкой текста ТЗ, а не пересмотром продуктового решения. +Геометрический контракт, non-scope, обработка конфликта с #28/#82 и модель +данных проверены по коду и корректны. + +Возврат в «ТЗ в работе» (`S3-spec`), цикл r1/4.