diff --git a/docs/reviews/SPEC-REVIEW-196-r1.md b/docs/reviews/SPEC-REVIEW-196-r1.md new file mode 100644 index 00000000..5af67c19 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-196-r1.md @@ -0,0 +1,230 @@ +# SPEC-REVIEW-196-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/196 +- **ТЗ под ревью:** тело issue #196 (лёгкий трек `small`, файл в `docs/specs/` + отсутствует — корректно, `docs/specs/196-*.md` не создан) +- **Роль:** ревьюер ТЗ (не автор), этап `S4-spec-review` +- **Трек:** лёгкий (`small`), лимит циклов ревью ТЗ — 2 (§4, §5 PROCESS.md) +- **Цикл:** r1/2 + +## Скоуп ревью + +ТЗ #196: room hover tooltip в View получает необязательную строку «средняя +влажность: N%» между температурой и LQI. Значение берётся из уже существующего +`_roomHum()` (tier-3 `hum_source` → среднее по HA-зоне из `_climate()`), +симметрично уже реализованной температуре. Device tooltip, room card/label +(влажность там уже есть), touch-эквивалент hover, backend, схема, миграция — +вне скоупа по тексту ТЗ. + +Ветки `issue/196-*` в репозитории ещё нет — ожидаемо: лёгкий трек, задача +только что вышла из аналитики в ревью ТЗ, продуктового кода не существует. +Гейты (`typecheck`/`test`/`build`, smoke) не прогонялись: на этом этапе им +неоткуда взяться (PROCESS.md §2.4/§8). + +## Как проверялось + +1. Прочитаны целиком `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (действующая + редакция, включая §1, §2.4, §5, §7.1, §8). +2. Прочитано тело issue #196 целиком и все три комментария: аналитика + владельца (ценность 4/10 · сложность 2/10 · P2 · `enhancement` · + поверхности `src/houseplan-card.ts` + i18n + smoke/docs · трек `small`, + `trivial` явно отклонён из-за нового i18n-ключа) и два «Взял:» на роли + аналитика/автора ТЗ. +3. Построчно сверены все технические утверждения раздела «Сейчас» и + «Контракт» с кодом `origin/dev` (`src/houseplan-card.ts`, 19891 строка): + - `_tip` / `_showTip()` — `houseplan-card.ts:713` и `:5758`: сигнатура + `(ev, title, meta, lqi?, temp?)`, поле `_tip` уже несёт `lqi?: number | + null; temp?: number | null`. Добавление `hum?: number | null` последним + параметром — прямое расширение без переупорядочивания существующих. + - Room hover передаёт `_showTip(e, name, area, lqi, this._roomTemp(r))` — + `houseplan-card.ts:15627-15633` (в ТЗ заявлено `:15621`, расхождение на + 6 строк из-за коммитов после написания ТЗ — не дефект, код найден и + совпадает буквально). + - Рендер тултипа — `houseplan-card.ts:15863-15874` (в ТЗ `:15860`, то же + смещение): строка `tip.temp` идёт первой (`if (this._tip.temp != null)`), + затем `tip.lqi` — вставка `hum` между ними ровно там, где просит + контракт п.3, синтаксически тривиальна (тот же паттерн `!= null ? html… + : nothing`). + - Device tooltip — `houseplan-card.ts:16729-16731`: вызывает `_showTip(e, + d.name, metrics)` — **без** `lqi`/`temp`/`hum` вовсе. Подтверждает + заявление контракта п.1 «existing device call остаётся семантически + неизменным»: добавление пятого необязательного параметра ничего здесь + не меняет, TS не потребует правки этого call site. + - `_roomTemp()` / `_roomHum()` — `houseplan-card.ts:16769-16781`: + функции буквально симметричны («tier-3 `temp_source`/`hum_source` → + иначе `_climate().get(area)`»). `_roomHum()` уже существует и уже + используется в room label (`:17085-17086`, `${hm}%` — тот же формат + значения, что просит ТЗ). Ссылка на реальный существующий helper, а не + на функцию, которую ещё нужно написать. + - `_climate()` — `houseplan-card.ts:16792-16802`: один кеш на `hass` + identity, комментарий на `:16790` дословно говорит «two, with humidity + on» (в ТЗ упомянут как `:16784`, тот же класс смещения) — подтверждает + п.7 контракта: чтение влажности не создаёт нового прохода по registry. + - `_noHover` — `houseplan-card.ts:5652-5653`, используется как guard + первой строкой `_showTip()` (`:5759`) — подтверждает контракт п.6: + `_showTip` уже отказывается строить тултип на no-hover устройствах, + новая строка не создаёт новую touch-поверхность просто потому, что + `hum` передаётся тем же вызовом. + - Значение уже целое: `areaClimateMap` округляет `hum` до `Math.round(...)` + (`src/devices.ts:1396`), `sourceValue()` для явного `entity:`/`device:` + источника — тоже `Math.round(v)` (`src/devices.ts:1252, humFor`). + Предположение ТЗ «значение не пересчитывается, используется готовый + результат `_roomHum()` с суффиксом `%`» корректно: дробного значения + физически не бывает, формат `${hum}%` без округления в тултипе не + потеряет точность. +4. Прочитан `src/i18n/en.json` и `ru.json`: `tip.temp_avg` (`:374` + `"average temperature:"` / `"средняя температура:"`) и `tip.lqi` + (`:150`) — предложенный `tip.hum_avg` (`"average humidity:"` / + `"средняя влажность:"`) следует тому же формату (двоеточие, строчная + буква, `average :`), новый ключ не выдуман по структуре. +5. Прочитан `docs/USER-GUIDE.ru.md`: строка 173 (таблица режимов) — + «подсказка показывает название, чистую площадь, температуру и LQI» — это + ровно то место, которое ТЗ обязано обновить (и заявляет это в разделе + Release), и ровно то место, что фиксирует нынешний пробел (влажности в + перечислении нет, хотя строка 128/275/1175 подтверждает, что влажность + уже показывается в room card). Строка 1310 «Комнатные подсказки основаны + на hover» — существующее, не создаваемое этим ТЗ ограничение, + распространяется на новую строку тем же образом, что и на текущие. +6. Прочитан `docs/TOUCH-SUPPORT.md` целиком. Правило «Documentation rule» + (`Touch editor: …`) относится к «New editor feature specifications» — + тултип комнаты живёт в View, не в редакторе, правило не применяется + механически. Контракт «fully supported View» (строка 31-32: «provide a + touch path for essential information that desktop exposes through + hover») для влажности не создаёт нового риска: `disp.labelHum` уже кладёт + влажность в room label (`houseplan-card.ts:17084-17086`), который виден + без hover — то есть на touch влажность уже доступна отдельным путём, + ТЗ не ухудшает существующий (уже документированный как ограничение, + `USER-GUIDE.ru.md:1310`) hover-only статус тултипа для area/temp/LQI. +7. Проверено, что `docs/specs/196-*.md` не существует (`ls docs/specs/` — + пусто по маске) — соответствует лёгкому треку. +8. Проверено соответствие `docs/SCOPE.md`: задача закрывает J5 «Room climate + at a glance» (persona home admin/household, View), не расширяет + out-of-scope списки, не касается lock invariant, не создаёт нового job'а. +9. Проверены оба существующих файла-кандидата для smoke: + `demo/smoke_ux_fixes.mjs` (`:21-27`) уже содержит инфраструктуру для + проверки `_tip.temp` и текстового содержимого `.tip` («средняя + температура»), то есть AC1-AC3 технически осуществимы без нового + тестового харнесса — паттерн для новой `hum`-проверки уже есть в файле. + `demo/srv/demo.html` не содержит фикстуры-сенсора влажности; но 31 файл + в `demo/` (в т.ч. `smoke_room_settings.mjs`, `smoke_sun.mjs` и др.) уже + инжектируют синтетические `hass.states` динамически в `page.evaluate` — + значит отсутствие штатной humidity-фикстуры не блокирует AC1/AC2, + разработчик может ввести сенсор тем же приёмом. Это техническое решение, + ТЗ корректно оставляет выбор конкретных имён/assertions разработчику + (раздел «Принятые технические предположения»). + +## Обязательные разделы для лёгкого трека (§5 PROCESS.md) + +| Раздел | Есть | Комментарий | +|---|---|---| +| Проблема | ✅ | Подтверждена построчным чтением кода (см. «Как проверялось» п.3): `_roomHum()` существует и используется в label, но не в tooltip — не догадка, факт | +| Контракт | ✅ | 7 пронумерованных пунктов, включая явную защиту от расширения device tooltip (п.1) и touch-поверхности (п.6) | +| AC1…ACn с доказательством | ✅ | 5 штук, у каждого назван способ (`smoke`/`unit`+`build`/«ревью кода») | +| Откат | ✅ | «Совместимость и откат»: revert одного коммита, без миграции, без cleanup | + +Дополнительно присутствуют разделы, не обязательные для лёгкого трека, но +усиливающие ТЗ: «Не входит в задачу» (явный анти-скоуп), «Файлы и проверки», +«Release, performance, security, visual», «Принятые технические +предположения» (3 пункта, все технические — не маскируют продуктовый вопрос). + +## Находки + +### Low-1 — AC2 частично привязывает `smoke`-доказательство к утверждению, проверяемому только кодом + +**Место:** тело issue #196, раздел «Критерии приёмки», AC2: «код tooltip +вызывает общий `_roomHum()`, а не дублирует resolver». + +Первая половина AC2 (значение override совпадает с настроенным +`hum_source`) действительно проверяется браузерным смоком через видимый +текст. Вторая половина («не дублирует resolver») — по природе утверждение о +структуре кода, а не о видимом поведении; смок не может отличить «вызвал +`_roomHum()`» от «скопировал ту же формулу в другом месте с тем же +результатом». Это не отменяет проверяемость: AC5 уже требует ревью кода на +отсутствие новых resolver'ов/scope creep, так что содержательно вторая +половина AC2 покрыта — реального пробела в доказательстве нет, только +неточная маркировка способа доказательства одним словом `smoke` на весь +пункт. + +**Обоснование severity:** Low — не блокирует, ничего не остаётся +непроверенным, дефект чисто редакционный (смешаны behavioral и structural +части одного AC под одной меткой доказательства). + +**Решение ревьюера:** снимаю без возврата ТЗ на правку. Код-ревью (AC5) +обязан явно подтвердить, что `_roomHum()` вызывается напрямую, без +дублирования резолвера — этот пункт настоящим решением фиксируется как +входящий в объём AC5, а не отдельно требующий правки текста ТЗ. + +## Что проверено и корректно + +- **Диагноз проблемы не голословен.** `_roomHum()` (`:16777-16781`) + дословно симметричен `_roomTemp()` (`:16769-16774`), уже используется в + room label (`:17085-17086`) и уже настраивается в диалоге комнаты + (`_renderRoomSource('hum')`, `:19717-19838`) — но не передаётся в + `_showTip()` при hover (`:15627-15633`). Пробел реален, не выдуман. +- **Соответствие `docs/SCOPE.md`:** закрывает J5, персона home + admin/household member, поверхность View; не расширяет lock invariant, + out-of-scope список, backend/schema/migration. +- **Контракт защищает device tooltip от регрессии** (п.1, подтверждено + реальным call site `:16729-16731`, который не передаёт `lqi`/`temp` + вовсе — пятый необязательный параметр `hum` для него ничего не меняет). +- **Touch-поверхность не ухудшается.** Room label уже показывает влажность + без hover (`disp.labelHum`, `:17084-17086`); hover-only статус самого + тултипа — существующее, уже задокументированное ограничение + (`USER-GUIDE.ru.md:1310`), не создаваемое этим ТЗ. Правило `Touch editor: + …` из `docs/TOUCH-SUPPORT.md` не применяется — это не editor-фича. +- **AC1-AC5 однозначны, каждый снабжён допустимым способом доказательства** + (`smoke`/`unit+build`/«ревью кода»); формат значения (`Math.round`, + целое число, суффикс `%`) подтверждён на уровне `devices.ts` — в тултипе + не появится дробное число, которое пришлось бы отдельно форматировать. +- **i18n-ключ следует существующей конвенции** (`tip.temp_avg` → + `tip.hum_avg`, тот же шаблон текста на обоих языках). +- **AC3 корректно защищает границы:** `null`/невалидный источник не + показывает строку (тот же паттерн `!= null`, что уже используют + `tip.temp`/`tip.lqi`), device tooltip не получает ложную влажность, + `_noHover` не создаёт тултип — все три случая проверены по коду и + логически вытекают из уже существующих guard'ов, не из нового кода, + который ещё предстоит написать правильно. +- **Откат тривиален** — один user-visible коммит, без миграции схемы/данных, + без cleanup. +- **Принятые технические предположения (3 пункта)** — все технические + (порядок параметра `_showTip`, отсутствие пересчёта формата, свобода + имён тестов), продуктовый вопрос под видом технического решения не + найден. +- **Не додумано скрытых предположений о поведении.** Ни одно утверждение + контракта не описывает поведение, которого нет в коде/канонических + документах и не помечено как assumption. + +## Чего не проверял + +- Реализацию — её нет: ветка `issue/196-*` в репозитории отсутствует, + продуктовый код не менялся (`git branch -r` не находит ветку). +- Гейты `typecheck`/`test`/`build`/smoke — не относятся к этапу ревью ТЗ + (PROCESS.md §2.4/§8), выполнить их не над чем. +- `python -m pytest tests_backend` — задача не касается backend ни кодом, + ни текстом ТЗ. +- Реальный визуальный результат (браузерный рендер строки влажности, + точное позиционирование `.tip`) — CSS/разметка ещё не написаны; + заявленное («тот же паттерн, что и temp/lqi») — это план, а не + исполненный факт, что и должно быть на этапе ТЗ. +- Точность числовых оценок аналитики (4/10, 2/10, P2) по существу — поле + владельца (PROCESS.md §2.2), не предмет ревью ТЗ. +- Возможность технического спора автор/ревьюер по разделу «Принятые + технические предположения» — не возникла, все три пункта разумны без + необходимости их оспаривать. + +## Вердикт + +Зелёный. High: 0, Medium: 0, Low: 1 (AC2 смешивает поведенческую и +структурную часть под одной меткой `smoke` — не оставляет пробела в +доказательстве, так как структурная часть уже покрыта AC5; снимается без +возврата ТЗ на правку, код-ревью проверяет обе половины). ТЗ подтверждено +построчным чтением кода `origin/dev`: каждое техническое утверждение +оказалось фактом (с точностью до нормального смещения номеров строк из-за +коммитов после написания ТЗ), продуктовых вопросов владельцу нет и не +додумано новых, AC1-AC5 однозначны и снабжены допустимыми способами +доказательства, откат тривиален, лёгкий трек применён корректно (новый +i18n-ключ и enhancement-тип верно исключают `trivial`, сложность/риск ≤3 и +одна поверхность верно подтверждают `small`). + +**Вердикт: зелёный · цикл r1/2 · High: 0 · Medium: 0 → в задаче · Документ: +docs/reviews/SPEC-REVIEW-196-r1.md**