diff --git a/docs/reviews/SPEC-REVIEW-487-r1.md b/docs/reviews/SPEC-REVIEW-487-r1.md new file mode 100644 index 00000000..2d47cba7 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-487-r1.md @@ -0,0 +1,228 @@ +# SPEC-REVIEW-487-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/487 +- **Этап:** ревью ТЗ (PROCESS.md §2.4) +- **Заход:** r1 · блокирующих циклов израсходовано 0 из 4 (лимит лёгкого трека + не применяется — трек полный) +- **Артефакт ТЗ:** `docs/specs/487-room-temperature-thresholds.md`, + зафиксирован коммитом `09328760` («docs: specify room temperature + thresholds», `Issue: #487 · User-Visible: no`, только `docs/**` — класс C). +- **Материал ревью:** тело issue #487, три комментария (аналитика S2, занятие + автора, готовность ТЗ), файл ТЗ на SHA `09328760`. + +## Скоуп + +Комнате разрешается задать собственный диапазон комфортной температуры +(`room.settings.temp_min/temp_max`), с независимым наследованием каждой +границы от `space.settings.temp_min/temp_max` (умолчания 20/25 °C). Единственный +потребитель — цвет температурной заливки пола комнаты (включая продолжение в +откосах проёмов). Room-card, tooltip, badge, источник температуры и алгоритм +«холодно/комфорт/жарко» не меняются. HA-helper-ссылки и глобальный уровень +диапазона — явно вне скоупа по решению владельца от 07.09.2026, зафиксированному +в теле issue. + +Трек — полный, обоснованно: новые compatibility-поля конфигурации и новый +UX-контракт наследования — два независимых критерия §5, каждого из которых +достаточно, чтобы трек не был `small`. Аналитик (комментарий S2) назвал оба. +Задача попадает в J5 «Room climate at a glance» из `docs/SCOPE.md` — фильтр +пройден, нового вида функциональности вне списка Core user jobs нет. + +## Как проверялось + +Ревью читает ТЗ состязательно: без устных пояснений автора, с проверкой +каждого фактического утверждения о текущем коде по самому коду — раздел 3 ТЗ +(«Подтверждённое текущее состояние») сплошь состоит из проверяемых утверждений +о конкретных функциях, и именно они подлежали проверке в первую очередь, а не +принятию на слово. + +Построчно сверено с деревом на `09328760` (HEAD этой ветки, код не менялся — +это чисто документационный коммит): + +| Утверждение ТЗ (§3) | Где проверено | Результат | +|---|---|---| +| `spaceDisplayOf()` проецирует `tempMin/tempMax` с default 20/25 | `src/logic.ts:1286-1327` | Совпадает дословно | +| `roomFillStyle()` нормализует границы через `Math.min/Math.max` | `src/logic.ts:1481-1490` (внутри режима `temp`) | Совпадает | +| `_resolvedRoomFills()` и `renderSpaceStatic()` передают всем комнатам одну пару пространства | `src/houseplan-card.ts:9276-9309` (`disp.tempMin, disp.tempMax` в цикле по `space.rooms`); `src/space-render.ts:497-505` (то же в `renderSpaceStatic`) | Совпадает — оба render path читают `disp.tempMin/tempMax`, не `room.settings` | +| `RoomCfg.settings` уже хранит `fill_mode/custom_fill/glow/temp_source/hum_source/name_scale/label_scale`, но не диапазон | `src/types.ts:9-22` | Совпадает, полей диапазона нет | +| `_saveRoomEdit()` бережно сохраняет неизвестные/future room settings через `{...previous}` | `src/houseplan-editor-runtime.ts:10959-10991` | Совпадает: `next = {...previous}`, известные поля точечно перезаписываются/удаляются | +| `_roomSettingsFromDialog()` строит настройки новой комнаты только из известных полей | `src/houseplan-editor-runtime.ts:10948-10957` | Совпадает: собирает `st` с нуля из 6 известных полей | +| Backend `ROOM_SCHEMA` разрешает extra-поля, диапазона в схеме нет | `custom_components/houseplan/validation.py:1267-1288` (`extra=vol.ALLOW_EXTRA`, список known settings без temp_min/max) | Совпадает | +| Plan-only проекция переносит только allowlist `_ROOM_DISPLAY_FIELDS` | `custom_components/houseplan/import_export.py:98-100` (`fill_mode, custom_fill, glow, name_scale, label_scale`) | Совпадает, диапазона в списке действительно нет — задача обязана его добавить (§7.5), и в скоупе это названо | + +Дополнительно проверено: + +- `roomFillModeOf()` (`src/logic.ts:1897-1904`) — существующий helper эффективного + режима заливки комнаты, на который ТЗ опирается при определении видимости + блока (§10.1 «действующий режим заливки — Температура»). Существует и делает + ровно то, что нужно. +- `strictNumber()` — общий парсер числового поля, используется в нескольких + местах (`src/houseplan-editor-runtime.ts:2914, 5793, 7945` и др.). ТЗ (§10.3) + предполагает reuse этого контракта для валидации — паттерн существует, это не + придуманная функция. +- `temp_source`/`hum_source` уже присутствуют в общем диалоге создания/ + редактирования комнаты (`src/houseplan-editor-runtime.ts:10916-10952`) — то + есть паттерн «один диалог на create/edit с полем комнаты» уже действует + сегодня для соседнего поля, а не изобретается для этой задачи. +- `docs/USER-GUIDE.ru.md` §13 «Заливки комнат и свет» → таблица «Наследование» + (строки 1335-1343) сегодня не упоминает температурные границы комнаты вовсе — + ТЗ корректно определяет это как пробел для закрытия release-артефактом (§17), а + не как расхождение с уже описанным контрактом. +- `docs/CONFIG-COMPATIBILITY.md` не содержит записи про temp-thresholds — ТЗ + корректно планирует новую запись (§17), не претендуя на то, что она уже + существует. +- i18n: проект поддерживает ровно `en/ru/de/fr` (`src/i18n/{en,ru,de,fr}.json`) — + ТЗ требует все четыре (§12), ничего не упущено и не придумано лишнего языка. +- `scripts/config-field-registry.mjs`, `scripts/config-schema.json` существуют и + имеют заявленную в ТЗ форму — ссылки на реальную инфраструктуру, не на + вымышленные скрипты. + +Ни одного утверждения о текущем поведении, которое не подтвердилось бы кодом, +не найдено. Раздел 3 ТЗ — это в буквальном смысле код-грамотный аудит, а не +пересказ по памяти. + +## Проверка §7.1 (обязательные разделы) и однозначности AC + +Все обязательные разделы присутствуют: сценарий (§1), что человек увидит до/ +после (§2, одной фразой, без терминов реализации — выполнено), проблема +(растворена в §3, но по существу присутствует — текущее поведение и его +недостаточность названы явно), скоуп и не-скоуп (§7, §8), контракт поведения +(§4, §6, §11), UX (§10), модель данных и миграция (§9), i18n (§12), AC1…AC12 с +доказательством (§13), план автотестов (§14), риски (§15), откат (§16), +release-артефакты (§17). + +AC1–AC12 пронумерованы, для каждого указан способ доказательства (unit / +backend / browser+golden) и — сверх формального минимума §7.1 — колонка «что +краснеет» (конкретный мутант или снятая защита), которая по факту в этом +процессе обязательна только на этапе код-ревью (§2.7 «Защитный AC доказывается +таблицей»), но её присутствие уже в ТЗ убирает половину работы будущего +код-ревью и делает критерии проверяемыми уже сейчас, а не только на словах. +Ни один AC не описывает решение только на словах «работает корректно» — +каждый называет конкретную входную комбинацию (полное/частичное/отсутствующее +наследование, перестановка границ, blank/zero/invalid, скрытие при смене +режима, parity рендеров, backend/compatibility, i18n). + +Проверка на «догадку, выданную за факт» (задача явно требуется процессом): +каждое утверждение о существующем поведении в §3 и §6 привязано к конкретной +функции/файлу и подтвердилось чтением кода (таблица выше). Новое поведение +(§4, §9-§12) последовательно отделено фразой «Принятые решения» и разделом 5 +«Принятые предположения» — то есть авторские технические решения размечены как +решения, а не незаметно подмешаны к описанию текущего состояния. Продуктовые +развилки (глобальный уровень диапазона, индикатор «свой диапазон», HA-helper +ссылки) не додуманы ТЗ, а взяты из уже состоявшихся решений владельца +07.09.2026, процитированных в теле issue дословно — свежих открытых +продуктовых вопросов ТЗ не содержит и не требуется задавать их отдельно. + +Единственное явно не проверяемое человеком-ревьюером (без запуска кода) +место — фактическое визуальное поведение узкого диалога (§10.4, AC8). Это +корректно вынесено на browser smoke + reviewed golden, а не описано как +самоочевидный факт. + +## Находки + +### Low — расхождение с уже принятой формулировкой «наследовать» (в скоупе, не блокирует) + +`docs/specs/487-room-temperature-thresholds.md` §10.2 вводит текст кнопки +сброса «Как в пространстве». Действующий i18n-ключ `fill.inherit` (используется +для аналогичного смысла — опции «унаследовать от пространства» в выборе режима +заливки комнаты, `src/houseplan-editor-runtime.ts:13926`) уже переведён как +«Как у пространства». Строка ТЗ не является ошибкой (это новая кнопка для +нового явления, а не переиспользование существующего ключа), но два почти +идентичных по смыслу элемента интерфейса с разной формулировкой создают +объективно и неопределённость терминологии, а не языковую вариативность. + +**Решение ревьюера:** не блокирует зелёный вердикт. Автор реализации решает +свободно — либо переиспользует существующую формулировку «Как у пространства» +для нового элемента ради консистентности словаря интерфейса, либо оставляет +предложенную и коротко фиксирует причину. Технический выбор строки локализации +не относится к продуктовым вопросам §7.1 и не требует владельца. + +Других Low, Medium или High находок нет. + +## Что проверено и корректно + +- Соответствие `docs/SCOPE.md`: задача закрывает J5, не расширяет и не создаёт + новый Core user job, не задевает out-of-scope список. +- Наследование двухуровневое, независимое по каждой границе, без глобального + уровня и без HA helper-ссылок — совпадает с решениями владельца 07.09.2026 из + тела issue. +- Единственный потребитель диапазона — цвет заливки; room-card/tooltip/badge + явно исключены и в §4.6, и в §8, и в AC11 (с названным способом падения — + появление нового climate-pass или влияние на нетемпературный fixture). +- «Одно число — один источник» (требование PROCESS.md §8) выполнено намеренно: + §11 явно запрещает читать локальные пороги внутри `roomFillStyle()`, требует + ровно один вызов `roomTempRangeOf()` за кадр и запрещает расхождение между + full View, static card и dialog preview; AC7 отдельно доказывает parity. + Здесь ТЗ не просто не создаёт дефект «второе число без синхронизации» — оно + проектирует резолвер именно для того, чтобы такой дефект стал структурно + невозможным. +- Совместимость: §9 корректно описывает forward/backward поведение (null + трактуется как «наследовать» и не пишется текущим UI, legacy-конфиги без + полей не мигрируют, plan-only allowlist получает два новых поля без утечки + HA-идентичности) и соответствует стилю уже существующих записей + `docs/CONFIG-COMPATIBILITY.md`. +- Откат (§16) корректно называет асимметрию: frontend откатить можно всегда, + backend обязан продолжать принимать поле в течение заявленного окна + совместимости, иначе откат стал бы разрушающим — это тот самый принцип, + на котором уже стоят все записи `CONFIG-COMPATIBILITY.md`. +- Release-артефакты (§17) перечисляют оба changelog, оба User Guide, обновление + `config-schema.json`/registry и reviewed golden — полностью по чек-листу DoR + (PROCESS.md §2.5). +- Трейлеры коммита ТЗ (`Issue: #487`, `User-Visible: no`) корректны для + документационного коммита без изменения поведения; запись в + `docs/specs/README.md` — одна строка, без дублирующего словаря статуса + (PROCESS.md §7.3 п.1 не нарушен). + +## Чего не проверял и почему + +- **Гейты `typecheck`/`test`/`build`/`check-docs` не гонялись.** Диапазон + коммита ветки — только `docs/specs/487-room-temperature-thresholds.md` и + `docs/specs/README.md` (класс C, `git show --stat 09328760`); `src/**` и + `custom_components/**` не тронуты, код не менялся с последнего зелёного + Validate на `dev`. Гонять дорогие гейты ради документационного коммита без + единой строки продуктового кода было бы прогоном ради прогона — сам ТЗ-этап + этого не требует (§2.4 PROCESS.md), а `check-docs.mjs` реагирует только на + диффы `src/**`, которых здесь нет. +- **Browser smoke / golden / backend pytest не гонялись** — по той же причине: + на этапе ТЗ нет кода, который они могли бы проверить. Это станет предметом + код-ревью, когда появится реализация; уже сейчас в ТЗ для каждого AC назван + конкретный смок/golden-сценарий и мутант — их станет чем проверять на + следующем этапе. +- **Не проверялась связь с #56 и #68 по факту кода** (наследование `custom_fill`, + паттерн подсказки «?») глубже, чем нужно для оценки + консистентности: ТЗ ссылается на них как на прецедент паттерна, а не как на + зависимость, которую эта задача меняет — их код не тронут и не должен быть + тронут этим ТЗ. +- **Не оценивалось качество будущей реализации** — ТЗ не описывает код, а + описывает контракт; реализация и её защитные тесты — предмет код-ревью. + +## Вердикт + +Зелёный. High: 0. Medium: 0. Low: 1 (в скоупе, не блокирует, решение — за +автором реализации). + +--- + +## Материал раунда + +- Ветка: `origin/issue/487-room-temperature-thresholds` +- SHA: `09328760e6259da94aac83dff38009a641917e32` +- Дерево ТЗ: `docs/specs/487-room-temperature-thresholds.md` (483 строки, тот же + SHA) +- Команда поиска материала при разрыве SHA: + `git log --all --find-object= -- docs/specs/487-room-temperature-thresholds.md` + +--- + + + +## Материал раунда + +- Ветка: `issue/487-room-temperature-thresholds`, коммит `09328760e625` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `eb4d00d27a3e2a3d403e2b68b5298bdbc3f74aa7` + ``` + git log --all --format='%H %T' | grep eb4d00d27a3e + ``` +- ТЗ `docs/specs/487-room-temperature-thresholds.md`, блоб `d1d33e782d3edd6fd7e393c6e372a7d76439d05e` + ``` + git log --all --find-object=d1d33e782d3edd6fd7e393c6e372a7d76439d05e -- docs/specs/487-room-temperature-thresholds.md + ```