diff --git a/docs/reviews/CODE-REVIEW-317-r1.md b/docs/reviews/CODE-REVIEW-317-r1.md new file mode 100644 index 00000000..f1b6b2e0 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-317-r1.md @@ -0,0 +1,199 @@ +# CODE-REVIEW-317-r1 + +- Issue: [#317](https://github.com/Matysh/houseplan-card/issues/317) — «Метрики под названием комнаты и тултип комнаты не показывают температуру и влажность, хотя датчики в комнате есть» +- Этап: code (S7-code-review) +- Заход: r1 · блокирующих циклов израсходовано 0 из 4 +- Диапазон: `git log --oneline origin/dev..HEAD` = `f19ed10f` (ТЗ), `b3df25ab` (SPEC-REVIEW, зелёный), `6c9752f5` (реализация, проверяемый SHA) +- ТЗ: `docs/specs/317-room-climate-placement.md`, SPEC-REVIEW-317-r1 — вердикт зелёный, High 0, Medium 0 +- Ревьюер: Claude (сессия код-ревью), состязательно, без пояснений автора + +## Скоуп ревью + +Один коммит реализации `6c9752f5`, User-Visible: yes, оба changelog внутри +того же коммита. Диф: `src/devices.ts` (новый `roomClimateMap`/`roomClimateKey`, +`areaClimateMap` — тонкая совместимая обёртка), `src/houseplan-card.ts` +(`_roomTemp/_roomHum` читают room-aware key, внешний gate `_renderRoomLabel` +больше не блокирует area-less комнату), `src/space-render.ts` (hosted Static +получил тот же resolver вместо `areaTemp`), `src/logic.ts`/`src/types.ts` +(только комментарии, контракт `room_id` не меняется), `test/devices.test.mjs` +(+6 unit-тестов), `demo/smoke_room_climate_placement.mjs` (новый), правки +`scripts/mutation-gate.mjs`, `scripts/smoke-links.mjs`, docs +(ARCHITECTURE/CHANGELOG×2/TESTING/USER-GUIDE×2) и синхронный ребилд бандлов. + +Первый код-ревью раунд — разбор полный, без разделов «Унаследовано»/«Закрытие +раунда» (они появляются с r2). + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md`, тело issue #317 и все + комментарии (`gh issue view 317`), `docs/USER-GUIDE.ru.md`, + `docs/ARCHITECTURE.md` (раздел «Room climate is one pass…»). +2. Прочитан ТЗ `docs/specs/317-room-climate-placement.md` целиком и + SPEC-REVIEW-317-r1 (зелёный, Low-находки — терминология HA Area/HA-зона и + формат доказательства AC8 — сняты автором ревью ТЗ, повторно не поднимаю). +3. Построчно сверен `git diff origin/dev...HEAD` с контрактом §6–§10 ТЗ: + - `src/devices.ts:1386-1408` (`roomClimateKey`, `markerClimateTarget`) — + `markerClimateTarget` требует именно `marker.area === null` вместе с + `space`+`room_id` для local-room target — это тот же точный предикат + `manualRoomWithoutArea`, что уже использует `resolveExplicitMarkerPlacement` + (`src/devices.ts:1065-1067`) для визуального размещения. Контракт не + изобретён заново, а согласован с существующим прецедентом — соответствует + заявлению ТЗ §6.2; + - `src/devices.ts:1432-1463` — приоритет `entityTargets → deviceTargets → + entity.area_id → device.area_id` совпадает с порядком §6.2 (1)-(4) + построчно; + - `src/devices.ts:1460` — новый gate `isRegistryEntryEnabled` (уже + существующий хелпер из `ha-binding-status.ts`, переиспользован, не + написан заново) отсекает HA-disabled entity/device — доказывает AC4; + - `src/houseplan-card.ts:11956-11970` (`_roomTemp/_roomHum` на + `roomClimateKey(this._spaceModel()?.id, r)`) и `:12095` + (`_renderRoomLabel` внешний gate теперь `disp.labelTemp || disp.labelHum + || (disp.labelLqi && r.area) || disp.labelLight`) — воспроизводит именно + тот дефект, который описан в §3 ТЗ и в issue (внешний gate раньше + блокировал area-less комнату независимо от включённых `disp.labelTemp/Hum`); + - `src/space-render.ts:203-215` — hosted Static построен на том же + `roomClimateMap`/`roomClimateKey`, а не на прежнем отдельном `areaTemp` + (импорт `areaTemp` убран из файла) — единственный источник числа между + View и hosted Static (AC8), а не два параллельных резолвера; + - `src/space-render.ts:429-431` — hosted Static не рисует metric-строки + (`rlm`) вообще, только имя комнаты; humidity/tooltip там не появлялись и + не появились — не расширяет незаявленный скоуп; + - `docs/ARCHITECTURE.md`, `docs/USER-GUIDE(.ru).md`, `docs/TESTING.md`, + оба `CHANGELOG` — обновлены тем же коммитом, формулировки соответствуют + §11 ТЗ и не вводят новую терминологию сверх `docs/USER-GUIDE.ru.md`. +4. Прогнаны дешёвые гейты и целевые smoke/mutation на SHA `6c9752f5` (зелёного + Validate на этом SHA нет — см. ниже). Полный список и результаты — в + разделе «Гейты». +5. По каждому AC1–AC11 найдено прямое доказательство в коде/тесте (таблица + ниже), а не принято на слово автора. + +## Гейты — что прогнал сам и с каким результатом + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | чисто, без вывода | +| Unit | `npm test` | `tests 1479 · pass 1478 · fail 0 · skipped 1` | +| Build | `npm run build` | success, `dist` собран | +| Bundle sync | `npm run bundle:sync` | пересборка не оставляет diff (`git status --porcelain` пуст после) — три копии (`dist`, `custom_components/houseplan/frontend`, `demo/srv/assets`) идентичны | +| Bundle budget | `npm run bundle:budget` | initial View 256127 B gzip, запас 25873 B до бюджета 282000 B | +| Docs fingerprint | `node scripts/check-docs.mjs` | `Documentation checks passed (7 files, 10 external links)` | +| any-gate | `node scripts/no-new-any.mjs --base origin/dev --head HEAD` | «Новых any нет» (101 добавленная строка в 5 файлах) | +| Whitespace | `git diff --check origin/dev...HEAD` | пусто | +| Smoke-select | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | прямое совпадение: `smoke_climate_once.mjs`, `smoke_climate_temp.mjs`, `smoke_cover_tap.mjs` (по символу `Marker`), `smoke_room_climate_placement.mjs` | +| Smoke | `node demo/smoke_room_climate_placement.mjs` | все 10 проверок `true`, `OK` | +| Smoke | `node demo/smoke_climate_once.mjs` | `OK` (one-pass не растёт от room count — AC9) | +| Smoke | `node demo/smoke_climate_temp.mjs` | `OK`, включая `avgWithOption`/`avgStillCorrect` | +| Smoke | `node demo/smoke_cover_tap.mjs` | `OK` — совпадение по `Marker` оказалось шумом (в diff `Marker` тронут только doc-комментарием в `types.ts`), но раз инструмент назвал его прямым — прогнал | +| Mutation | `node scripts/mutation-gate.mjs --id=room-climate-ignores-marker-placement` | `тест покраснел, как обязан` — `smoke_room_climate_placement.mjs` ловит мутанта, который игнорирует marker placement (AC10) | +| Visual | `npm run golden:verify` | все сценарии `passed`, включая `lighting-fill-temp-axis-split-dark`, `lighting-temp-glow-*` (единственные golden-сцены с `fillMode: temp`) — нет визуальной регрессии на существующих HA-Area комнатах | +| Single-source | `node --test test/single-source-numbers.test.mjs` | 3/3 passed (механическая часть; смысловая часть — см. ниже) | + +Не прогонял: +- `npm run invariants` — diff не трогает рёбра комнат, записи толщины, + `layout`, `open_spans`; `marker.space/room_id` здесь используются только как + вход в climate-агрегатор (новая пара-ключ для карты температур), а не как + геометрическая ссылка. Подсистема стен/толщины не затронута ни одной + строкой diff — гейт неприменим, а не пропущен. +- `python -m pytest tests_backend` — diff не трогает `custom_components/**/*.py` + (только пересобранные frontend-ассеты). +- Полная матрица `demo/smoke_*.mjs` и полный `golden:capture`/предрелизный + набор — не требуются на этапе ревью (§8), выбор `smoke-select` плюс явно + названные в AC7/AC9 smoke уже покрывают три поверхности и hosted Static; + `golden:verify` прогнал явно, так как diff меняет видимый рендер + (temp-fill/labels area-less комнаты). +- Performance-бенчмарки (`benchmark:*`) — в AC не названы, `smoke_climate_once` + уже доказывает отсутствие роста числа проходов. + +## AC — доказательство + +| AC | Требование | Доказательство | Статус | +|---|---|---|---| +| AC1 | Sensor из Area A с live marker в Area B голосует только в B, при совпадении — один раз | `test/devices.test.mjs` «explicit device placement moves one climate vote between HA areas» | ✅ прогнан, зелёный | +| AC2 | `space + room_id + area:null` даёт климат area-less комнате | unit «a hidden real sensor supplies an area-less House Plan room» + `smoke_room_climate_placement` (`localAutomatic`, `labelShowsAutomatic`) | ✅ прогнаны | +| AC3 | Exact `entity:` приоритетнее parent `device:` только для этой entity | unit «exact entity placement wins over its parent marker only for that entity» | ✅ прогнан | +| AC4 | Hidden участвует; removed/HA-disabled/unavailable — нет; tombstone #262 сохраняется | unit «removed and HA-disabled bindings cannot feed a local room» + `hidden: true` в двух других unit-тестах | ✅ прогнан | +| AC5 | `use_climate_temp` следует effective placement, air/non-air и округление не изменены | unit «opted climate current_temperature follows manual placement» + существующие climate-тесты (`npm test` зелёный) | ✅ прогнан | +| AC6 | Explicit room source выше automatic, invalid не даёт silent fallback | `sourceValue()` не изменён diff'ом (проверено чтением — вызовы `_roomTemp/_roomHum`/`roomTemperature` идут в `sourceValue` первым тиром без изменений сигнатуры/тела) + `smoke_room_climate_placement.explicitSourceWins` | ✅ проверено чтением + smoke | +| AC7 | Label/tooltip/fill area-less комнаты — один result, перенос marker обновляет все | `smoke_room_climate_placement`: `labelShowsAutomatic`, `tooltipSharesAutomatic`, `localTempFill`, `oldLabelDropsMovedValue`, `oldTempFillRemoved` — все `true` | ✅ прогнан | +| AC8 | View/kiosk и hosted Static — одинаковый result, config не мутирует | код-ревью (не автотест, как и указала SPEC-REVIEW): `space-render.ts` использует тот же `roomClimateMap/roomClimateKey`, импорт `areaTemp` убран; `smoke_room_climate_placement.staticLocalTempFill/staticOldFillRemoved` подтверждают на практике; `git diff` не касается кода записи config | ✅ проверено чтением + smoke | +| AC9 | Один registry-проход на snapshot, не растёт с комнатами | `demo/smoke_climate_once.mjs` → `OK` | ✅ прогнан | +| AC10 | Мутант, игнорирующий marker placement, ловится тестом | `mutation-gate.mjs --id=room-climate-ignores-marker-placement` → «тест покраснел, как обязан» | ✅ прогнан | +| AC11 | typecheck/unit/build/bundle/docs/smokes зелёные | таблица «Гейты» выше | ✅ прогнан | + +## Находки + +Blocking (High): нет. + +В скоупе (Medium): нет. + +Вне скоупа (Medium → issue): нет. + +### Low (снимаю с записью, без правки) + +1. **Нет юнит-теста на дублирующиеся live-маркеры одного binding в + `roomClimateMap`.** Комментарий в коде (`src/devices.ts:1441-1442`) + заявляет «тот же детерминированный first-match contract, что и + `buildDevices()`», и реализация (`!entityTargets.has(ref)) ... set(...)`) + действительно берёт первое вхождение по порядку массива — то же самое, что + `deletePlanMarkerRecords`/дедуп в `buildDevices` делают для дублей + binding'ов в остальном проекте. Сценарий «два live-маркера с одинаковым + `binding` на разные комнаты» — уже известная малослучаемая аномалия + данных (не новая, не введена этим diff), и mutation-guard #317 нацелен не + на неё, а на сам факт учёта placement. Снимаю без правки: риск и + вероятность малы, поведение читаемо и соответствует уже принятому в + проекте паттерну; если владелец захочет явную регрессию на дубли — + отдельная маленькая правка теста, не блокирующая эту задачу. + +## Что проверено и корректно + +- Диагноз и весь контракт §6–§10 ТЗ воспроизведены в коде без расхождений; + `markerClimateTarget` использует тот же предикат `area === null` + `space` + + `room_id`, что и существующий `resolveExplicitMarkerPlacement` — не новый + параллельный контракт для той же persisted-структуры. +- Приоритет источников (marker exact → marker parent → registry entity → + registry device → нет цели) реализован в заявленном порядке и покрыт unit + + mutation. +- HA-disabled/removed/tombstone (#262) поведение не регрессировало: + переиспользован существующий `isRegistryEntryEnabled` и неизменённый + `removedPlanBindings`. +- Единственность источника числа (AC7/AC8): `_roomTemp/_roomHum` — общий путь + для label/tooltip/fill в View; hosted Static куда раньше использовал другой + резолвер (`areaTemp`) — теперь единый `roomClimateMap`, импорт старого + убран, что устраняет потенциальное расхождение, а не добавляет новое. +- `golden:verify` подтверждает отсутствие визуальной регрессии на всех + существующих `fillMode: temp` сценариях (HA-Area комнаты ведут себя как + раньше — эффективный target совпадает с их текущим `room.area`). +- Persisted schema/model version не менялись (только doc-комментарии в + `types.ts`/`logic.ts`), миграции нет — совпадает с §9 ТЗ и явно проверено + `git diff` на `src/types.ts`. +- Трейлеры коммита `6c9752f5`: `Issue: #317`, `User-Visible: yes`; оба + changelog (`docs/CHANGELOG.md`, `docs/CHANGELOG.ru.md`) правлены в том же + коммите с одной строкой на #317 — соответствует требованию. +- Документация (ARCHITECTURE/USER-GUIDE ru+en/TESTING) обновлена по существу, + без новых незаявленных терминов; `docs/images/screenshots.json` обновлён + тем же коммитом, `check-docs.mjs` зелёный. + +## Чего не проверял + +- Ручное тестирование на живом HA-инстансе не входит в цикл ревью (см. + инструкцию) — заменено чтением diff + browser smoke на реальном DOM карточки + (`demo/smoke_room_climate_placement.mjs` реально монтирует + `houseplan-card`/`houseplan-space-card` в браузере через `serve.mjs`, не + мокает). +- `npm run invariants` и полный `demo/smoke_*` набор — обоснование в разделе + «Гейты» выше. +- Backend Python — не тронут. +- Поведение `sourceValue()` изнутри — не переисследовал построчно, так как + функция не изменена diff'ом; доверяю существующему покрытию (не входит в + дельту этой задачи). + +## Вердикт + +Реализация соответствует зафиксированному в ТЗ контракту, все AC1–AC11 +подтверждены автотестом, mutation-guard'ом или чтением с явной пометкой, все +дешёвые гейты и релевантные smoke/golden/mutation прогнаны мной лично и +зелёные, trailers и changelog корректны. Blocking-находок нет, Medium нет ни в +скоупе, ни вне скоупа. Одна Low-находка снята с записью (не блокирует). + +Вердикт: зелёный · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 0 · +Документ: docs/reviews/CODE-REVIEW-317-r1.md