Files
houseplan-card/docs/reviews/SPEC-REVIEW-196-r1.md
2026-08-19 10:23:47 +00:00

21 KiB
Raw Permalink Blame History

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 <metric>:), новый ключ не выдуман по структуре.
  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