diff --git a/docs/reviews/SPEC-REVIEW-29-r1.md b/docs/reviews/SPEC-REVIEW-29-r1.md new file mode 100644 index 00000000..5ab123bb --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-29-r1.md @@ -0,0 +1,191 @@ +# SPEC-REVIEW-29-r1 + +- Issue: https://github.com/Matysh/houseplan-card/issues/29 +- Этап: spec (PROCESS.md §2.4) +- Заход: r1 · блокирующих циклов израсходовано 0 из 4 +- ТЗ: `docs/specs/029-device-inbox-lifecycle.md` +- SHA материала ревью: `a6ce1ae7e534fa4b41fcc0ab93e5ed95a693c4c1` (ветка `issue/29-device-inbox-lifecycle`) +- Трек: обычный (не `small`), лимит циклов ревью ТЗ — 4. + +Это первый заход ревью по этой задаче — предыдущего вердикта в issue нет, +раздел «Унаследовано» не применяется, разбор полный. + +## Скоуп разбора + +Проверено: + +1. `docs/SCOPE.md` — соответствие job'ам J4/J6, отсутствие выхода за + продуктовую рамку (out-of-scope список, standing rules). +2. `AGENTS.md` и `PROCESS.md` §2.4, §7.1 — формальные требования к ТЗ и + формат вердикта. +3. Тело issue #29 и все комментарии, включая повторную актуализацию анализа + от 2026-08-28 и решение владельца по Q1. +4. `docs/USER-GUIDE.ru.md` (терминология «Скрытые и деактивированные», + «Редактор устройств», раздел про скрытие/удаление) и `docs/FILTERING.md` + (канонический контракт hidden/removed/tombstone/entity-ownership). +5. Исходный код: `src/houseplan-card.ts` (`_bindingCandidates`, + `new_device_ids`, `_maybeRebuildDevices`, `.slice(0, 200)` дважды), + `src/ha-binding-status.ts` (`HaBindingStatus.kind`), `src/types.ts` + (`marker.hidden`, `marker.removed`), `src/i18n/ru.json` и `en.json`. +6. Существующие тесты, упомянутые в AC11/§16: `test/devices.test.mjs`, + `test/ha-binding-status.test.mjs`, `test/device-presentation*.test.mjs`, + `demo/smoke_hidden_flag.mjs`, `demo/smoke_binding_picker.mjs` — + существуют, содержимое `smoke_hidden_flag.mjs` прочитано целиком. +7. `docs/CONFIG-COMPATIBILITY.md` — беглая проверка на отсутствие + противоречий заявлению «нет новых persisted-полей». + +Ревью — на этапе ТЗ: продуктового кода нет, автотесты не запускались (их +ещё не существует), гейты §8 к этому этапу не относятся. + +## Метод проверки + +Каждое фактическое утверждение ТЗ о текущем поведении продукта +(«До», §3, §7, §8, §9, §10.1) сверено с кодом или каноническим документом, а +не принято на слово автора. Отдельно проверено, есть ли в тексте +утверждение о поведении, которого нет ни в одном документе и которое не +помечено как предположение (раздел 21 ТЗ). + +## Находки + +### M1 (Medium, в скоупе) — исчезновение призрачного показа скрытых/HA-disabled маркеров на плане не названо как продуктовое решение + +**Файл:** `docs/specs/029-device-inbox-lifecycle.md`, §10.1 (строки 216–222). + +Сегодня редактор устройств может показать скрытые и HA-disabled маркеры +прямо на плане, в их реальной позиции, призраками — режим переключается +кнопкой «Скрытые и деактивированные» (`docs/FILTERING.md` строки 84–91, +`docs/USER-GUIDE.ru.md:178` — «доступны скрытые маркеры», перетаскиваются; +подтверждено в `demo/smoke_hidden_flag.mjs`, где ghost рендерится в +`_setMode('devices')` с `_showHidden = true` и клик по нему открывает +диалог). Это единственный способ увидеть, ГДЕ на плане сидит скрытое или +деактивированное устройство, не отменяя его скрытость. + +ТЗ прямо убирает этот режим: «Скрытые markers больше не рисуются поверх +плана постоянным локальным режимом: доступ к ним даёт каталог» (§10.1). +Но каталог (§10.2–10.4) не даёт эквивалента: `Find` явно доступен «только +если marker реально отрисовывается» (§10.4), а для строк категории +«Скрытые» и для «На плане, временно не отображается из-за HA status» +primary/secondary действия — это «Показать»/«Настроить», не позиция на +плане. Значит, чтобы увидеть, где стоит скрытый маркер, администратору +придётся сначала его показать (что меняет конфиг), посмотреть, и при +необходимости скрыть обратно — вместо непосредственного просмотра. + +Это видимое пользователю изменение объёма функциональности («какая +персона что видит и делает» — ровно тот класс вопросов, который согласно +PROCESS.md §7.1 задаётся владельцу или явно фиксируется как принятое +предположение в §21). В тексте ТЗ оно подано как самоочевидный +технический побочный эффект объединения кнопок, а не как решение, +которое можно оспорить: обоснование «устраняет состояние панели, +неочевидное после возврата в редактор» — это плюс нового дизайна, но оно +не адресует потерю прямого просмотра позиции. + +**Не High**, потому что: обходной путь существует (Показать → посмотреть/ +перетащить → Скрыть), это не потеря данных и не поломка AC — просто +непроверенное продуктовое допущение, которое дёшево закрыть на этом этапе. + +**Как закрыть в этом же цикле (на выбор автора):** либо явно вынести это в +блок §21 как предположение, которое ревьюер/владелец может оспорить, с +описанием обходного пути; либо задать это владельцу одним пакетным +вопросом с предложенным дефолтом (например: «Find для скрытой/disabled +строки временно подсвечивает позицию на плане не снимая hidden» как +альтернативный дизайн); либо сознательно сохранить упрощённый вариант, но +явно назвать компромисс и обходной путь в §2 «До/После» и в §18 «Риски». + +### M2 (Medium, в скоупе) — устаревающая строка i18n не включена в план обновления + +**Файлы:** `src/i18n/ru.json:673`, `src/i18n/en.json:673` (`marker.hide_tip`). + +Текущий текст подсказки при скрытии маркера дословно ссылается на кнопку, +которую это ТЗ удаляет: RU — «Вернуть его можно через кнопку «Скрытые и +деактивированные» в редакторе устройств»; EN — `Restore it through "Hidden +and disabled" in the device editor`. §10.1 заменяет обе кнопки («Добавить» +и «Скрытые и деактивированные») одной кнопкой «Устройства» (`devbar.add`, +`devbar.show_all` перестают существовать в UI в текущем виде). + +Раздел 14 (i18n) перечисляет только **новые** ключи и не содержит пункта +«обновить существующие ключи, ссылающиеся на удаляемые элементы +интерфейса». Если реализовать ТЗ как написано, `marker.hide_tip` останется +нетронутым и после релиза будет указывать пользователю нажать +несуществующую кнопку — конкретный, проверяемый дефект, а не гипотетический. + +**Как закрыть:** добавить в §14 явный пункт «`marker.hide_tip` (en+ru) +обновляется, чтобы указывать на новую точку входа «Устройства»» (и +проверить, нет ли других строк с той же ссылкой — быстрый `grep` по +`show_all`/«Скрытые и деактивированные» в `src/i18n/*.json` показывает, +что это единственная пара строк такого рода, кроме самих `devbar.*` +/`title.show_all`, которые и так меняются по §10.1). + +Обе находки Medium, в скоупе задачи (правки в самом файле ТЗ) — по +PROCESS.md §2.4/§2.7 они не создают отдельный issue (решение владельца +2026-08-19, #202) и возвращают ТЗ автору с жёлтым вердиктом. + +## Что проверено и признано корректным + +- **Формальные разделы §7.1** — сценарий, «что человек увидит до/после», + проблема, скоуп/не-скоуп, контракт поведения, UX, модель данных и + миграция, i18n, AC1…AC11 с указанием способа доказательства, план + автотестов, риски, откат, release-артефакты — все присутствуют. +- **AC1–AC11** однозначны и у каждого назван способ доказательства + (unit/smoke/golden/review); ни один не описывает недоказуемое поведение. +- **Q1 владельца** (классификация автообнаруженного видимого устройства + без marker) корректно отражена в §7.2 (категория `on_plan`) и в AC3 — + соответствует решению владельца в комментарии issue. +- **Заявления о текущем поведении, использованные как база для «До», + подтверждены кодом и канонoм**, а не додуманы: + - `_bindingCandidates` в `src/houseplan-card.ts:14032` — существующий + eligibility-код, который ТЗ предлагает извлечь в общий helper (§12); + не выдумка. + - `HaBindingStatus.kind` (`active`/`ha_disabled`/`orphaned`/`unverified`) + в `src/ha-binding-status.ts:14-17` — точное совпадение с таблицей §7.3. + - `marker.hidden`/`marker.removed` в `src/types.ts:118,121` — существуют, + семантика совпадает с `docs/FILTERING.md`. + - Жёсткий кап на 200 элементов в существующих списках кандидатов + (`src/houseplan-card.ts:14106`, `19114` — `.slice(0, 200)`) — + подтверждает описанную в §8.1/AC9 проблему, которую каталог обязан не + унаследовать. + - `duplicate_name_area` действительно устарела: `docs/FILTERING.md:145` + прямо говорит «Duplicate names are still numbered», не скрываются. + - `settings.new_device_ids` фильтруется от уже скрытых id при seed + (`src/houseplan-card.ts:3679–3684`) — подтверждает §8.2 «Первично + отфильтрованный скрытый кандидат не получает badge, как и сегодня». + - Терминология «Добавить», «Скрытые и деактивированные», «Правила + иконок» — точное совпадение с `src/i18n/ru.json:455-457` и + `docs/USER-GUIDE.ru.md`. +- **Технические предположения (§21)** промаркированы явно и корректно + отделены от продуктовых решений — кроме пробела, описанного в M1. +- **Не входит (§6)** корректно исключает #44 (discovery-настройки), #126 + (смена area), #109 (multi-channel), bulk-операции, историю/графики, + touch-паритет — всё по `docs/SCOPE.md` и `docs/TOUCH-SUPPORT.md`, без + расширения скоупа. +- **Откат (§19)** реалистичен: нет новых persisted-полей и версии модели, + откат — вернуть старые кнопки; совместимость с созданными через «Скрыть + из списка» обычными `hidden:true` маркерами сохраняется. +- Названные тестовые артефакты существуют: `test/devices.test.mjs`, + `test/ha-binding-status.test.mjs`, `test/device-presentation.test.mjs`, + `test/device-presentation-policy.test.mjs`, `demo/smoke_hidden_flag.mjs`, + `demo/smoke_binding_picker.mjs`, `scripts/smoke-select.mjs` — ни один не + является выдумкой. + +## Чего не проверял + +- Не запускал никакие гейты (`typecheck`/`test`/`build`/смоки) — на этапе + ТЗ продуктового кода нет, это не применимо (§8 относится к + код-ревью). +- Не проверял `docs/ARCHITECTURE.md` и `docs/CANVAS.md` целиком постранично + — только точечно то, что касается заявленных в ТЗ архитектурных решений + (pure resolver, eligibility helper); полный аудит этих документов не + требуется для ревью ТЗ. +- Не оценивал производительность реализации (её ещё нет); проверил только, + что заявленная асимптотика (§12, §17) не противоречит существующему коду + (`.slice(0, 200)` факт капа подтверждён). +- Не проверял golden/скриншот-инфраструктуру предметно — она релевантна + коду, а не ТЗ. + +## Вердикт + +Полностью выполненные формальные требования ТЗ (§7.1) не отменяют жёлтый +вердикт: две находки Medium в скоупе (M1, M2) — реальные пробелы, оставить +их «в тексте ревью» без правки запрещено §12 PROCESS.md. High-находок нет. + +**Вердикт: жёлтый.** ТЗ возвращается автору на правку M1 и M2; после +правки — новый заход ревью по дельте (PROCESS.md §2.10).