mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
claude[bot]
@@ -0,0 +1,102 @@
|
||||
# SPEC-REVIEW-274-r1
|
||||
|
||||
- Issue: [#274](https://github.com/Matysh/houseplan-card/issues/274) — «Беспроводной выключатель на плане приглушён, хотя превью того же маркера обещает нейтральную подложку»
|
||||
- Этап: spec (PROCESS.md §2.4)
|
||||
- Заход: r1 · блокирующих циклов израсходовано 0 из 4
|
||||
- ТЗ: `docs/specs/274-wireless-controller-presentation-parity.md`
|
||||
- Коммит: `9b76427` (ветка `issue/274-wireless-controller-parity`, совпадает с текущим `HEAD`)
|
||||
- Класс изменения этого коммита: C (документация) — `docs/specs/274-...md` + `docs/specs/README.md`, продуктовый код не тронут.
|
||||
|
||||
## Скоуп ревью
|
||||
|
||||
Первый заход, разбор полный (иное не требуется — предыдущего вердикта нет). Проверено:
|
||||
|
||||
1. Соответствие `docs/SCOPE.md` — какую строку Core user jobs закрывает задача.
|
||||
2. Обязательные разделы ТЗ по PROCESS.md §7.1 и чек-лист DoR §2.5.
|
||||
3. Однозначность и проверяемость AC1…AC8, способ доказательства у каждого.
|
||||
4. Достоверность технического диагноза — сверка построчных ссылок на код, приведённых в ТЗ и в issue, с фактическим `src/device-presentation.ts` и `src/houseplan-card.ts`.
|
||||
5. Отсутствие догадки, выданной за факт, и корректность обращения с продуктовыми вопросами владельца (issue поднимал два вопроса; проверено, закрыты ли они уже принятым контрактом #251, а не додуманы).
|
||||
6. Согласованность с каноническим предшествующим ТЗ `docs/specs/251-controller-target-availability.md`, на контракте которого строится #274.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
- Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком.
|
||||
- Прочитано тело issue #274 и оба комментария (аналитика владельца, хендофф ТЗ на ревью).
|
||||
- Прочитан сам файл ТЗ целиком (317 строк) и связанный `docs/specs/251-controller-target-availability.md` целиком.
|
||||
- `git show 9b76427 --stat` и `git diff d83b0c5..9b76427 --stat` — подтверждено, что коммит трогает только `docs/specs/**`, класс A/B не задет.
|
||||
- Построчная сверка диагноза: `src/device-presentation.ts:559-235,574-597,600-635`, `src/houseplan-card.ts` (`_renderDeviceSnapshot`, вызовы preview-резолва вокруг строки 20279), `src/hp-device-preview.ts:171`, `src/i18n/en.json` (`marker.preview.reason.*`).
|
||||
- Проверено существование файлов, на которые ссылается план тестов: `test/devices.test.mjs`, `test/device-presentation.test.mjs`, `test/device-toggle.test.mjs`, а также уже существующий `demo/smoke_device_preview_parity.mjs` (готовая инфраструктура именно для сравнения plan/preview одного и того же маркера).
|
||||
|
||||
## Гейты
|
||||
|
||||
Диапазон коммита — только `docs/specs/**`, класс C, продуктовый код не менялся. `typecheck`/`test`/`build`/`check-docs`/смоки/инварианты в этом раунде **не гоняются**: ни один их предмет (src, тесты, конфигурация геометрии) в диффе не участвует, а сам этап — ревью ТЗ, а не кода. Это решение, а не пропуск: перечисляю его явно.
|
||||
|
||||
## Проверка диагноза по коду (не на слово автору)
|
||||
|
||||
ТЗ и issue называют точные строки текущего кода как основание диагноза. Все они подтвердились при чтении:
|
||||
|
||||
- `src/device-presentation.ts:561` — `if (visual.availability === 'unavailable') return 'unavailable';` (причина для preview) — совпадает дословно.
|
||||
- `src/device-presentation.ts:583` — `else if (visual.availability === 'unavailable') classes.push('unavail');` (класс для плана) — совпадает дословно.
|
||||
- `src/device-presentation.ts:218-235` — комментарий и тело `controllerAvailability()` — совпадает дословно, включая формулировку «Controls are not controller evidence…».
|
||||
- `resolveDevicePresentation()` (строка ~600-635) действительно вызывает `controllerAvailability(hass, d)` только для `sourceKind === 'controls'`, то есть один и тот же чистый резолвер используется и планом, и preview — значит расхождение объясняется входом (`hass`/`d.entities`), а не веткой логики. Это ключевое утверждение ТЗ (§3), и оно подтверждается.
|
||||
- `src/houseplan-card.ts` — план читает `this._renderDeviceSnapshot?.presentations.get(...)` (предвычисленный, иммутабельный снэпшот), тогда как ветка preview (около строки 20279) вызывает `resolveDevicePresentation(this._planHass, previewDevice, {...})` заново, где `previewDevice` — результат `_markerPreviewDevice(d)`. Второй верхнеуровневый путь подтверждён так же, как заявляет ТЗ.
|
||||
|
||||
Диагноз ТЗ не является догадкой, выданной за факт: там, где точная причина ещё не доказана (какое именно поколение registry/roster вызывает расхождение), текст прямо говорит «без живого runtime dump не доказан» и запрещает реализации выбирать ветку по догадке (§3, §14 п.1 промаркирован как предположение). Это ровно то поведение, которого требует процесс.
|
||||
|
||||
## Продуктовые вопросы владельца — закрыты корректно, не додуманы
|
||||
|
||||
Issue поднимал два вопроса владельцу («что видеть для кнопки без своего состояния» и «считается ли `unknown` у `event.*` признаком недоступности»). ТЗ (§4) не додумывает ответ, а показывает, что оба уже даны решением по #251: правило 5 контракта #251 («доступность по любому живому own entity state, диагностика участвует, controls — нет») отвечает на оба вопроса дословно, и это решение уже принято владельцем (не техническое, не спорное). Проверено, что #251 действительно формулирует именно эти пункты (см. `docs/specs/251-controller-target-availability.md` §4 п.1,5 и §6.1). Трактовка ТЗ верна: #274 — это регрессия/рассинхронизация уже принятого контракта, а не новое продуктовое решение, поэтому пачка вопросов владельцу здесь не нужна.
|
||||
|
||||
## Проверка AC1…AC8
|
||||
|
||||
Все восемь пронумерованы, для каждого указан способ доказательства (`unit`/`smoke`/`golden`/`mutation`/`review`), как того требует DoR §2.5:
|
||||
|
||||
- **AC1** — точная фикстура (event=unknown, battery=100, LQI=164, update=off, group=off) и точный ожидаемый результат (`available`, `neutral`, без `unavail`, `lqiText=164`). Однозначно, проверяемо unit-тестом.
|
||||
- **AC2** — сравнение DOM плана и preview для того же сохранённого маркера без правок; для этого уже существует инфраструктура — `demo/smoke_device_preview_parity.mjs` (сравнивает `.dev` на плане и в `hp-device-preview` по классам/иконке/значению). AC не называет файл по имени, но выбор файла — техническое решение автора (§7.1), а не пробел ТЗ.
|
||||
- **AC3** — матрица переходов `off → on → unavailable → off`, однозначные ожидания для каждого шага.
|
||||
- **AC4** — перечислена конкретная минимальная матрица поколений (registry arrival, state tick, save/rebind, continuity hold/candidate/commit, reconnect), критерий «одно поколение — одно лицо» сформулирован проверяемо.
|
||||
- **AC5** — явно требует не ослаблять негативный контракт #251 (event-only binding, all-unknown case), ссылается на существующую table-driven matrix для расширения, а не переписывания.
|
||||
- **AC6** — parity View/kiosk/Static/preview в light/dark; корректно относит semantic golden к предрелизному циклу («локальный baseline не принимается»), что соответствует общему правилу PROCESS §8/§13.
|
||||
- **AC7** — два конкретных, содержательных мутанта (не source-regex) с указанием, какие AC обязаны покраснеть при каждом. Это именно то, что требует «дисциплина падающего теста».
|
||||
- **AC8** — гейты реализации, корректно разделяет локальный цикл и предрелизный цикл.
|
||||
|
||||
Ни один AC не сформулирован расплывчато; у каждого есть точка отказа, по которой можно судить о провале.
|
||||
|
||||
## Обязательные разделы (PROCESS.md §7.1) и DoR (§2.5)
|
||||
|
||||
Все обязательные разделы §7.1 присутствуют по существу: сценарий/персона (§1), что человек увидит до/после (§2), проблема и подтверждённые факты (§3), контракт поведения (§4, §6), scope/не-scope (§5), UX/touch/compatibility (§7), критерии приёмки с доказательством (§8), план автотестов (§9), риски (§12), откат (§13), release-артефакты (§11). Продуктовых открытых вопросов нет, что явно зафиксировано (§4).
|
||||
|
||||
Один пункт чек-листа DoR §2.5 — «перечислены затронутые файлы и модули» — не оформлен отдельным списком. Функции и файлы фактически названы по тексту (`buildDevices()`, `controllerAvailability()`, `RenderDeviceSnapshot`, `test/devices.test.mjs`, `test/device-presentation.test.mjs`, документация в §11), но сведены не в один список. Не считаю это блокирующим: сама граница диагноза (§3) сознательно не зафиксирована («без runtime dump не доказан»), и заранее заявленный список конкретных production-файлов был бы ровно той вставленной вперёд догадкой, которую процесс запрещает. Снимаю как Low с этой записью, а не превращаю в Medium — фиксировать список файлов раньше, чем найден точный источник рассинхронизации, значило бы гадать.
|
||||
|
||||
## Находки
|
||||
|
||||
### Low — «До»-описание в §2 использует внутренний термин вместо простой фразы (снято)
|
||||
|
||||
`docs/specs/274-wireless-controller-presentation-parity.md:35` — фраза «план приглушает выключатель классом `unavail`» вставляет имя CSS-класса в раздел, который по формату должен быть «одной фразой без терминов реализации» (PROCESS.md §7.1). Не блокирует: это репродукция симптома вплотную к тексту issue, вторая часть того же предложения («показывает зелёный LQI 164, а preview обещает нейтральную подложку») уже передаёт смысл на пользовательском языке, и точное имя класса скорее помогает второй-ревьюер/автору однозначно опознать баг. Снимаю как Low с этой записью, править не обязательно.
|
||||
|
||||
### Low — DoR-пункт «затронутые файлы и модули» не оформлен отдельным списком (снято)
|
||||
|
||||
Разобран выше — снят с записью «список файлов сознательно не даётся до red production-path теста, это соответствует принципу „граница диагноза не додумывается“».
|
||||
|
||||
High/Medium находок нет.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- Диагноз построен на подтверждаемых номерах строк, все сверены с фактическим кодом на `HEAD` — не пересказ автора.
|
||||
- Контракт #274 не переопределяет и не ослабляет уже принятый контракт #251 (явная негативная AC5 плюс явный список «не входит»).
|
||||
- Продуктовые вопросы владельца из тела issue закрыты ссылкой на уже принятое решение #251, а не выданы за новую догадку.
|
||||
- AC1…AC8 пронумерованы, у каждого — однозначный ожидаемый результат и способ доказательства; мутанты (AC7) содержательны.
|
||||
- Компатибилити/миграция/i18n/touch/perf/security разделы присутствуют и корректно говорят «без изменений» там, где это действительно так (persisted config, backend, touch-жесты, i18n-ключи).
|
||||
- Release-артефакты (§11) перечисляют оба changelog, ARCHITECTURE.md, USER-GUIDE.{md,ru.md}, TESTING.md — соответствует правилу «документация в том же коммите».
|
||||
- Трассируемость issue ↔ ТЗ ↔ индекс (`docs/specs/README.md`) на месте в обе стороны.
|
||||
- Коммит ТЗ несёт корректные трейлеры (`Issue: #274`, `User-Visible: no`) для чисто документационного класса C изменения.
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- Не гонял `typecheck`/`test`/`build`/`check-docs`/смоки/инварианты — диапазон коммита не содержит класса A/B (только `docs/specs/**`), поэтому эти гейты не относятся к предмету этого раунда ревью ТЗ.
|
||||
- Не проверял глубину архитектуры `RenderDeviceSnapshot`/continuity commit barrier построчно — это территория код-ревью после того, как красный production-path тест по AC1/AC2 будет написан; на этапе ТЗ важно, что граница явно не додумана, а не то, где именно лежит фикс.
|
||||
- Не связывался с владельцем — продуктовых вопросов, требующих его решения, не осталось.
|
||||
|
||||
## Вердикт
|
||||
|
||||
Зелёный. ТЗ технически обосновано, ссылки на код проверены и совпадают, AC однозначны и проверяемы, продуктовые вопросы закрыты ссылкой на уже принятое решение, а не догадкой. Обе Low-находки сняты с записью в этом документе.
|
||||
Reference in New Issue
Block a user