mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-01 12:18:51 +00:00
claude[bot]
committed by
Sergey Matyunin
Sergey Matyunin
parent
903bc0a345
commit
e5bda27cc3
@@ -0,0 +1,237 @@
|
||||
# SPEC-REVIEW-251-r1
|
||||
|
||||
- Issue: [#251](https://github.com/Matysh/houseplan-card/issues/251) — «доступность контроллера не наследуется от управляемой цели»
|
||||
- Этап: ТЗ на ревью (PROCESS.md §2.4)
|
||||
- Заход: r1 · блокирующих циклов израсходовано 0 из 4 (первый раунд, разбор полный)
|
||||
- Артефакт ТЗ: `docs/specs/251-controller-target-availability.md` (355 строк), запись в `docs/specs/README.md:130`
|
||||
- Ветка: `issue/251-controller-target-availability`
|
||||
- Ревьюер получил issue и ТЗ без устных пояснений автора; ниже — независимая проверка по коду `dev`.
|
||||
|
||||
## Скоуп ревью
|
||||
|
||||
Диапазон `origin/dev..HEAD` — один коммит `ba0c787` («docs(spec): separate
|
||||
controller and target availability»), трейлеры `Issue: #251` /
|
||||
`User-Visible: no` на месте, класс C (документация), правка сама по себе не
|
||||
требует `User-Visible: yes`. Изменены только `docs/specs/251-*.md` и
|
||||
`docs/specs/README.md`. Продуктовый код (`src/**`) не тронут — это ожидаемо
|
||||
для этапа ТЗ.
|
||||
|
||||
Предмет ревью: полный текст ТЗ против §7.1 PROCESS.md (обязательные разделы),
|
||||
против диагноза в теле issue и трёх комментариев владельца/аналитика, и против
|
||||
реального кода `src/device-presentation.ts` / `src/device-toggle.ts` /
|
||||
`src/houseplan-card.ts` — верификация того, что заявленные в ТЗ факты о
|
||||
текущем поведении не догадка, а точное описание существующего кода.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` §2.4/§2.9/§7.1/§7.2,
|
||||
`docs/USER-GUIDE.ru.md` (разделы «Жесты маркера», «Действия», «Визуальные
|
||||
состояния устройств», §12), `docs/TOUCH-SUPPORT.md`.
|
||||
2. Прочитано тело issue #251 и все 6 комментариев (диагноз владельца,
|
||||
актуализация аналитики, claim-комментарий, вопрос Q1, решение владельца,
|
||||
хендофф на ревью).
|
||||
3. Гейты этапа spec:
|
||||
- `node scripts/check-docs.mjs --external` → `Documentation checks passed
|
||||
(7 files, 10 external links)` — green;
|
||||
- `node scripts/process-gate.mjs --range origin/dev..HEAD --issues` →
|
||||
`гейт пройден, предупреждений 0` — green.
|
||||
Оба совпадают с тем, что заявил автор в хендоффе.
|
||||
4. Построчная сверка технических утверждений ТЗ с кодом:
|
||||
- `resolvePresentationSources()` в `src/device-presentation.ts:233-369` —
|
||||
подтверждено, что для маркера с `controls` `visualSources`/`samples`
|
||||
строятся исключительно из `lights` (controls-derived + owned light
|
||||
sources) и **никогда** не читают `d.entities` устройства-контроллера
|
||||
(battery/LQI/event туда не попадают в принципе, кроме ветки `alarm` в
|
||||
`criticalSources`). Это точное описание дефекта, не догадка.
|
||||
- `resolveDevicePresentation()` (там же, 580-711) — `visual =
|
||||
combineVisualSamples(sources.samples)`, класс `unavail` берётся из
|
||||
`visual.availability === 'unavailable'` (строка 564) раньше, чем
|
||||
`working`/`open` — подтверждает утверждение ТЗ §6.1 о приоритете
|
||||
`unavail` над `working`.
|
||||
- `src/device-toggle.ts`: `SkippedToggleTarget{ref, entityId, name, reason}`
|
||||
(37-58), `toggleOperation()` (763-767) возвращает `null`, когда нет
|
||||
`command`/`operation` — подтверждает опору AC3/AC4/AC5 на реальную,
|
||||
уже существующую классификацию, а не на новый API.
|
||||
- `resolveOwnEntity()`/`reasonForSingle()` (405-497) — для одиночной цели
|
||||
`noneReason` уже мапит `missing → 'unavailable'`, `skippedTargets`
|
||||
сохраняет причину и имя — ровно то, что нужно для локализованного
|
||||
сообщения без нового запроса к HA (заявление аналитика подтверждено).
|
||||
- `_clickDevice()` в `src/houseplan-card.ts:4930-4979` — строка 4932,
|
||||
`if (!initial || !toggleOperation(initial)) return; // ... quiet no-op`
|
||||
— это именно тот тихий `return`, который ТЗ называет дефектом второй
|
||||
части (п.3 «Актуализации аналитики»). Подтверждено чтением, не
|
||||
исполнением.
|
||||
- Токст toasta: `src/houseplan-card.ts:16473,17069` —
|
||||
`<div class="toast" role="alert" aria-live="assertive">` — ТЗ §7.1/§8
|
||||
точно описывает существующую разметку (после первичной проверки я
|
||||
нашёл в коде другой, необязательный `role="status" aria-live="polite"`
|
||||
у `_renderRecoveryOverlay()`, 3389-3399 — это другой компонент,
|
||||
оверлей восстановления связи, не toast; ложная тревога снята).
|
||||
- `confirm.unavailable_targets` (`src/i18n/ru.json:722`,
|
||||
`src/houseplan-card.ts:19805`) — существующий паттерн «Недоступно:
|
||||
{count}» без словоформ; новый текст ТЗ («Цели недоступны: {names}»)
|
||||
грамматически не зависит от числа и не наследует эту проблему —
|
||||
формулировка ТЗ безопасна для RU-множественного числа.
|
||||
- `isControllable()` (`src/logic.ts:1657-1658`) ограничивает прямые
|
||||
ссылки `controls` доменами `light`/`switch`; `secureEntity()`
|
||||
(`device-toggle.ts:209-218`) относится к `lock`/`alarm_control_panel`/
|
||||
охраняемым классам `cover` — по коду смешение `secure` с
|
||||
`unavailable/missing` в одной группе `controls` технически достижимо
|
||||
только через `marker:`-алиас на **другой** маркер, помеченный
|
||||
`is_light: true` (`_controlCandidates`, `houseplan-card.ts:19975-19992`
|
||||
фильтрует кандидатов именно так) — узкий, нетипичный путь конфигурации.
|
||||
- Виртуальные маркеры (`devices.ts:1246-1265`) всегда получают
|
||||
`entities: []` — значит строка матрицы §6.1 «virtual controller без
|
||||
own entities» — это не частный случай, а единственно возможный для
|
||||
`virtual: true`; formulировка §5 «виртуальные контроллеры... как
|
||||
всегда доступные» не создаёт скрытого противоречия с матрицей.
|
||||
- `demo/smoke_controls.mjs`, `test/device-presentation.test.mjs`,
|
||||
`test/device-toggle.test.mjs` существуют — ссылки AC1/AC3/AC4 на них
|
||||
не повисают в воздухе.
|
||||
5. Сверка терминологии с `docs/USER-GUIDE.ru.md`: текущая документированная
|
||||
формулировка «Если все сущности, реально описывающие маркер, недоступны,
|
||||
маркер бледнеет» (строка 855-857) и «Бледный маркер | Данные недоступны...
|
||||
| Все рабочие сущности маркера unknown, unavailable или отсутствуют»
|
||||
(871-875) — это ровно контракт, который ТЗ №251 сознательно меняет
|
||||
(разделяет «рабочие сущности» на own-controller и target). ТЗ п.14
|
||||
(«Release-артефакты») правильно требует правку `USER-GUIDE.md`/`.ru.md` в
|
||||
том же коммите реализации — иначе документация и код разойдутся, как уже
|
||||
было отмечено process-риском в самом ТЗ (риск 6).
|
||||
|
||||
## Находки
|
||||
|
||||
Обе найденные точки — низкой серьёзности; ни одна не блокирует и не
|
||||
требует возврата автору. Записываю решение ревьюера по каждой (PROCESS.md
|
||||
§3.8: Low либо правится, либо снимается решением ревьюера с записью).
|
||||
|
||||
### L1 — не проговорено соответствие toast'а формулировке владельца «стандартными средствами HA»
|
||||
|
||||
Первый комментарий владельца в issue: «...выводить корректное сообщение...
|
||||
**стандартными средствами HA** (как пишется, что соединение с HA потеряно)».
|
||||
ТЗ §7.1/§8 отвечает на это локальным тостом самой карточки House Plan
|
||||
(`role="alert" aria-live="assertive"`), а не нативным механизмом HA
|
||||
(например, событием `hass-notification`, которым сама HA показывает баннер
|
||||
о потере соединения). ТЗ не проговаривает это расхождение и не отмечает
|
||||
выбор как намеренную интерпретацию.
|
||||
|
||||
**Почему не Medium:** в кодовой базе уже существует устоявшийся паттерн —
|
||||
все аналогичные «действие не выполнено» сообщения (`toast.error`,
|
||||
`toast.tap_target_changed`, `toast.no_entity`, `toast.ha_disabled_action`,
|
||||
`toast.run_target_missing`) используют именно локальный тост карточки, а не
|
||||
нативный HA-механизм. Новое сообщение, реализованное иначе, было бы
|
||||
единственным исключением и визуально разошлось бы с остальной картой.
|
||||
Фраза владельца «как пишется, что соединение с HA потеряно» по контексту
|
||||
описывает регистр/тон сообщения («это системное объяснение, а не тихая
|
||||
ошибка»), а не требует буквально вызвать HA-нативный баннер; последующие
|
||||
три коммента владельца (Q1 и решение) не возражают против уже
|
||||
предложенного тостового решения аналитика.
|
||||
|
||||
**Решение ревьюера:** снимаю как Low. Прошу автора добавить в §7.1 или §17
|
||||
одну строку, явно фиксирующую, что «стандартные средства HA» реализуются
|
||||
локальным тостом карточки по образцу уже существующих `toast.*`-сообщений —
|
||||
это превратит implicit-решение в явно записанное предположение и закроет
|
||||
дорогу к спору на код-ревью.
|
||||
|
||||
### L2 — не решён смешанный `secure` + `unavailable` случай в группе `controls`
|
||||
|
||||
§7.1 формулирует условие тоста как «все пропуски объясняются `unavailable`,
|
||||
`missing` или `ha-disabled`» и отдельно говорит, что no-op из-за `secure`
|
||||
сохраняет существующее (тихое) поведение. Не описан случай, когда в одной
|
||||
группе `controls` одновременно есть и `secure`-цель, и `unavailable`/
|
||||
`missing`-цели, при нулевом исполняемом множестве: он не подходит под
|
||||
условие «все пропуски объясняются unavailable/missing/ha-disabled» (раз
|
||||
среди них есть secure), но и не подходит под «no-op вызван secure»
|
||||
(раз среди них есть не-secure причины). Естественная реализация на базе
|
||||
существующего `noneReason` (`device-toggle.ts:536-537`, который различает
|
||||
только `'secure'` при **всех** secure и `'configured-targets-missing'` во
|
||||
всех остальных случаях) молча покажет тост и для этого смешанного случая —
|
||||
поведение, о котором ТЗ прямо не высказывается.
|
||||
|
||||
**Почему не Medium:** по коду смешение достижимо только через `marker:`-
|
||||
алиас на другой маркер с `is_light: true`, а не через прямую ссылку на
|
||||
`lock.*`/`alarm_control_panel.*` (те отфильтрованы `isControllable()` уже на
|
||||
этапе выбора в редакторе). Обычный охраняемый `cover` (garage/door/gate)
|
||||
почти никогда не выставляется как `is_light: true`, так что путь требует
|
||||
нетипичной ручной конфигурации. Независимо от исхода замок/охранная панель
|
||||
не получает command — инвариант `SCOPE.md` («никогда не по тапу на плане»)
|
||||
не затрагивается ни при каком прочтении.
|
||||
|
||||
**Решение ревьюера:** снимаю как Low. Достаточно одной уточняющей строки в
|
||||
§7.1 или §16 (риски): смешанная группа classифицируется так же, как
|
||||
«все пропуски unavailable/missing/ha-disabled» — тост показывается, имена
|
||||
secure-целей в тексте тоста не участвуют отдельным исключением. Не блокирует
|
||||
разработку; можно поправить в рамках реализации без повторного цикла ревью
|
||||
ТЗ.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- Обязательные разделы §7.1 PROCESS.md присутствуют и в правильном порядке:
|
||||
сценарий/персона (§1) → что человек видит до/после (§2) → диагноз (§3) →
|
||||
решение владельца (§4) → скоуп/не-скоуп (§5) → контракт presentation (§6) →
|
||||
контракт действия (§7) → UX/a11y/touch (§8) → данные/compat/migration (§9) →
|
||||
perf/security (§10) → AC1-AC9 с доказательством (§11) → план реализации и
|
||||
автотестов (§12) → mutation guards (§13) → release-артефакты (§14) →
|
||||
откат (§15) → риски (§16) → явный блок принятых технических предположений
|
||||
(§17).
|
||||
- Продуктовые разделы 1-2 отвечают на «какая персона/поверхность/момент» и
|
||||
«что человек видит одной фразой без терминов реализации» — оба
|
||||
недвусмысленны.
|
||||
- Единственный продуктовый вопрос (Q1 — как считать доступность самого
|
||||
контроллера при отсутствии device-level state в HA) задан владельцу пачкой,
|
||||
с предлагаемым default, `blocked` был выставлен и снят после ответа —
|
||||
процесс §2.3/§7.1 соблюдён буквально. «Продуктовых вопросов не осталось»
|
||||
(§4 ТЗ) подтверждается тем, что все решения §4 действительно восходят к
|
||||
цитируемым словам владельца, а не к догадке автора.
|
||||
- Матрица §6.1 однозначна и проверяема: каждая строка задаёт конкретный вход
|
||||
(состояния own/target) и конкретный выход (availability/status/класс),
|
||||
привязана к AC1 и к перечисленному mutation guard
|
||||
(`controller-availability-follows-target`,
|
||||
`controller-diagnostics-do-not-prove-online`).
|
||||
- Скоуп/не-скоуп (§5) взаимно непротиворечивы и совпадают с решением
|
||||
владельца (нет нового бейджа, Glow не меняется, групповая семантика «any on
|
||||
→ all off» не меняется, secure-инвариант не трогается).
|
||||
- Модель данных/миграция (§9) — корректно «нет миграции»: правка чисто
|
||||
presentation/action, персистентная схема `marker.controls` не меняется,
|
||||
подтверждено тем, что все затронутые функции читают, а не переписывают
|
||||
`d.entities`/`d.controls`.
|
||||
- Все AC (1-9) пронумерованы и у каждого указан способ доказательства
|
||||
(`unit`/`smoke`/`golden`/«ревью кода»/config diff) — требование DoR §2.5
|
||||
выполнено.
|
||||
- i18n: точный RU/EN текст обеих реплик дан в §7.1; именование конкретных
|
||||
ключей оставлено автору как техническая деталь (не наблюдается
|
||||
пользователем) — это в рамках §7.1 PROCESS.md («именование... агенты
|
||||
решают сами»), не дефект готовности к разработке.
|
||||
- Touch/a11y (§8): не вводит новых жестов, использует существующий
|
||||
keyboard/tap путь и объявление screen reader — согласуется с
|
||||
`TOUCH-SUPPORT.md` (View/kiosk — гарантированная touch-поверхность) и не
|
||||
создаёт editor-специфичного расхождения.
|
||||
- Откат (§15) реалистичен: одна code revision, без обратной миграции данных.
|
||||
- Release-артефакты (§14) перечисляют оба changelog, `ARCHITECTURE.md`,
|
||||
оба `USER-GUIDE`, `TESTING.md` — учитывая, что USER-GUIDE.ru.md сейчас
|
||||
прямо документирует старый (заменяемый) контракт «Бледный маркер», это
|
||||
необходимое требование, а не избыточное.
|
||||
- Гейты spec-этапа (`check-docs`, `process-gate`) перезапущены ревьюером
|
||||
независимо и совпали с заявленными автором результатами.
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- Не запускал `npm run typecheck` / `npm test` / `npm run build` — диапазон
|
||||
`origin/dev..HEAD` не содержит изменений `src/**`/`test/**` (только
|
||||
`docs/specs/**`), эти гейты на этапе ТЗ не применимы к нулевому
|
||||
код-диффу; они станут обязательными в код-ревью реализации.
|
||||
- Не проверял `demo/smoke_controls.mjs` и `test/device-presentation.test.mjs`
|
||||
/ `test/device-toggle.test.mjs` на способность падать под будущими
|
||||
мутантами — на этапе ТЗ кода мутантов ещё нет; это станет предметом
|
||||
код-ревью (раздел §13 «Mutation guards» ТЗ уже задаёт, что именно должно
|
||||
покраснеть).
|
||||
- Не проверял golden/скриншот-провенанс — на этапе ТЗ `src/**` не менялся,
|
||||
`check-docs.mjs --external` уже подтвердил, что текущий provenance зелёный.
|
||||
- Не оценивал реальную Zigbee/HA-инсталляцию из исходного репорта (батарея
|
||||
100, LQI 164, группа из четырёх лампочек) — это внешний живой стенд
|
||||
владельца, недоступный ревьюеру; диагноз проверен по коду, а не повторным
|
||||
воспроизведением на инсталляции.
|
||||
|
||||
## Вердикт
|
||||
|
||||
High: 0 · Medium: 0 (два Low найдены и сняты решением ревьюера с записью
|
||||
выше, без возврата автору) → **зелёный**. ТЗ готово к разработке.
|
||||
Reference in New Issue
Block a user