docs: review document for #437

Issue: #437
User-Visible: no

docs/reviews/CODE-REVIEW-437-r1.md

@@ -0,0 +1,268 @@
+# CODE-REVIEW-437-r1
+
+Issue: [#437](https://github.com/Matysh/houseplan-card/issues/437) — конфигурируемая read-only сводная панель поверх плана.
+Материал: `git log --oneline origin/dev..HEAD` / `git diff origin/dev...HEAD` на SHA `96e07b9a0255eef3a55cd6c3be08198a0adcc7ba`.
+ТЗ: [docs/specs/437-summary-panel.md](../specs/437-summary-panel.md), принято ревью ТЗ r2 (зелёное, High 0/Medium 0).
+Этап: код-ревью, заход r1, лимит циклов 4/4 (полный трек).
+
+## Скоуп диффа
+
+82 файла, +5131/−424. Продукт: `src/summary-panel*.ts` (новые), точечные правки
+`houseplan-card.ts`/`houseplan-panel.ts`/`config-store.ts`/`types.ts`/`chrome.styles.ts`;
+backend — `validation.py`/`websocket_api.py`/`const.py` (новый namespace
+`settings.summary_panel`, версионирование, change-aware проверка ссылок).
+Тесты: `test/summary-panel.test.mjs`, `tests_backend/test_summary_panel.py`,
+`demo/smoke_summary_panel.mjs`, правка `test_support_package.py`,
+`test/styles-split.test.mjs`. Документация: оба changelog, USER-GUIDE ru/en,
+UX-MODES, TOUCH-SUPPORT, CONFIG-COMPATIBILITY, ARCHITECTURE,
+`scripts/config-field-registry.mjs`/`config-schema.json`. Бандл пересобран и
+синхронизирован в трёх копиях (класс D).
+
+Отдельно фиксирую для трассируемости: в теле issue и в ТЗ несколько раз
+записана «команда владельца: ТЗ → S5-ready, затем остановка, реализацию не
+начинать» (комментарии `5586184163`, `5586343543`, `5586611633`). Реализация
+всё же началась двумя минутами позже тем же account/committer'ом
+(`5586638954` → коммиты `Sergey Matyunin <s.matyunin@justbusiness.site>`).
+Поскольку это тот же git-identity, что и автор ТЗ и постановщик самой
+команды на остановку, трактую это как собственное решение владельца
+продолжить, а не как обход чужого запрета агентом; отдельного авторизующего
+комментария от третьей стороны не требуется по роли. Это наблюдение, не
+находка код-ревью — фиксирую, чтобы SHA-цепочка была прослеживаема.
+
+## Как проверялось — гейты
+
+Зелёного Validate на `96e07b9a` не найдено — прогнал сам.
+
+| Гейт | Команда | Результат |
+|---|---|---|
+| Типы | `npx tsc --noEmit` | pass, 0 ошибок |
+| Юниты | `npm test` | 2264 passed, 0 failed, 1 skipped (33.7s) |
+| Сборка + бандл | `npm run build && npm run bundle:sync` | pass; `cmp` подтвердил байтовое совпадение `dist/houseplan-card.js` ↔ `custom_components/.../houseplan-card.js` ↔ `demo/srv/assets/houseplan-card.js` |
+| any-гейт | `node scripts/no-new-any.mjs --base origin/dev --head HEAD` | pass, 1907 новых строк в 14 файлах, новых `any` нет |
+| Докскрины | `node scripts/check-docs.mjs` | RED — отпечаток скриншотов устарел. Ожидаемо и не блокирует ревью (PROCESS §8: это предупреждение на обычном push в `dev`, жёсткий гейт только у релиз-кандидата с `Release:`); пересъёмка остаётся обязанностью перед бетой |
+| Бюджет бандла | `node scripts/bundle-budget.mjs` | pass; initial View 291234 B / потолок 292000 B, запас 9832 B — ниже порога 15000 Б, скрипт сам печатает предупреждение и ссылку на трекнутый долг #367. Не новая находка этой задачи — существующий механизм уже сигналит владельцу, отдельно не завожу |
+| Смок-подбор | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | 73 «прямых совпадения» из 232 — почти все по широким символам самого `houseplan-card.ts` (`_mode`, `_config`, `_model`, `stopPropagation`), сигнал зашумлён. Прогнал точечно: новый `demo/smoke_summary_panel.mjs` (ниже) и `demo/smoke_kiosk.mjs`, единственный, где диф трогает конкретно изменённую функцию `_saveKioskScale` не по совпадению общего имени, а по факту рефакторинга её тела |
+| Новый браузерный смок | `node demo/smoke_summary_panel.mjs` | OK (перепрогнал сам, не только со слов автора) |
+| Существующий смок (флагован выше) | `node demo/smoke_kiosk.mjs` | **FAIL** — необработанное исключение, см. находку M1 |
+| Backend, чистый набор | `python -m pytest tests_backend/test_summary_panel.py -q` (voluptuous/pytest доустановлены локально) | 9 passed |
+| Backend, полный HA-harness | — | не прогонял: `.venv-backend` в этом окружении отсутствует (не облачный агент); доверяю отчёту автора (`.venv\Scripts\python.exe -m pytest tests_backend -q` → 396 passed, 3 skipped) с оговоркой — не перепроверено независимо |
+| Инварианты геометрии | `node scripts/model-invariants.mjs` | не прогонял: диф не пишет ни рёбра/толщину/`layout`/`marker.space`/`open_spans` — `totalCleanFloorAreaM2` только читает существующую геометрию для суммы площади, ничего не изменяет в модели |
+| golden | `npm run golden:verify` | не прогонял: overlay — DOM-sibling вне SVG/zoomwrap слоя (подтверждено чтением `summary-panel-style.ts`/`renderPanel`), существующий рендер плана не тронут; полный набор — предрелизный гейт |
+| performance | — | не прогонял: не названо блокирующим в AC для этого раунда, откладываю на пре-релиз по PROCESS §8; логику таймера/мемоизации проверил чтением (см. ниже) |
+
+## Находки
+
+Все — Medium, в скоупе задачи, чинятся в этой же итерации (без High это жёлтый
+вердикт, PROCESS §2.7). Ни одна не выходит за рамки #437, отдельных issue не
+завожу.
+
+### M1 — существующий смок `demo/smoke_kiosk.mjs` падает необработанным исключением на этом SHA
+
+**Файл:** `demo/smoke_kiosk.mjs:52` (`out.persisted = JSON.parse(localStorage.getItem('houseplan_card_kiosk_v1')).icon === 1;`).
+
+Диф меняет `_saveKioskScale` (`src/houseplan-card.ts`) так, что она целиком
+делегирует в `LoadedSummaryPanelRuntime.saveScale` → `saveLocal`, которая (по
+ТЗ §8.3, сознательно) пишет только в новый per-instance ключ
+`houseplan.summary-panel.v1:...` и **больше не пишет** в легаси-ключ
+`houseplan_card_kiosk_v1` («старый ключ не удалять и не переписывать» — это
+верно и явно требуется ТЗ). Продуктовое поведение корректно: я независимо
+проверил, что масштаб иконок и диалог размеров реально работают
+(см. «Проверено» ниже). Проблема в том, что старый смок, всё ещё
+утверждающий «после сохранения легаси-ключ обновился», не был обновлён вместе
+с рефакторингом, и падает `TypeError: Cannot read properties of null (reading
+'icon')`, потому что `localStorage.getItem('houseplan_card_kiosk_v1')`
+теперь `null` (ключ никогда не создавался в demo-профиле — легаси-seed
+срабатывает только если ключ уже существовал).
+
+**Чем краснеет:** воспроизведено напрямую — `node demo/smoke_kiosk.mjs`
+на `96e07b9a` падает необработанным исключением (см. таблицу гейтов),
+поэтому ни одна из последующих 4 проверок в этом файле (диалог размеров,
+карусель, пауза после касания, обычная карточка) в принципе не выполняется —
+файл прерывается на середине. `scripts/smoke-select.mjs` называл этот файл
+прямым совпадением по символу `_saveKioskScale`; PROCESS §8 требует именно
+такие смоки гонять до code-review — это не сделано.
+
+**Правка:** обновить assertion на новый контракт (например, прочитать
+per-instance ключ через `summaryLocalKey`, либо проверить, что легаси-ключ
+остался нетронутым, если он не существовал изначально) так, чтобы файл не
+падал и продолжал проверять оставшиеся пункты списка.
+
+### M2 — исключение «удалённого устройства» в подсчёте Q2 не имеет свидетеля
+
+**Файл:** `src/summary-panel-metrics.ts:25` и `:45` (`representedHaDeviceIds`).
+
+ТЗ §7.2/§12 явно требует численную фикстуру «removed binding исключён,
+explicit restored child возвращает ровно 1», и AC18 заявляет это как защиту.
+Я снял обе проверки `removed.devices.has(...)` по очереди (через-area путь и
+через-marker путь) и перезапустил `test/summary-panel.test.mjs` —
+оба раза все 12 тестов остались зелёными: фикстура в тесте
+(`#437 device total counts unique represented real HA device ids…`) не
+содержит ни одного маркера с `removed: true`, поэтому обе ветки исключения
+никогда не выполняются под тестом.
+
+**Чем краснеет:** мутация выполнена дважды, результат приведён (см. таблицу
+гейтов и вставки выше): `git diff` мутации доступен в истории этой сессии,
+после проверки файл возвращён в исходное состояние (`git status` чист).
+
+**Правка:** добавить в фикстуру маркер с `removed: true` (или эквивалент из
+`removedPlanBindings`) и явно восстановленную дочернюю entity, как в
+числовом примере ТЗ §12, чтобы тест ловил регрессию в любой из двух ветвей.
+
+### M3 — tap-target реордер/удаления в форме настроек меньше заявленных 44×44 CSS px
+
+**Файл:** `src/summary-panel-style.ts:197-207` (`.summary-editor-row button { min-width: 34px; height: 34px; }`).
+
+ТЗ §5.4 явно объявляет: «Touch editor: supported только для новой простой
+формы сводки» — то есть именно эта форма (а не только сам overlay и
+составной контрол в шапке) получает touch-контракт — и тут же: «Размер
+tappable зоны **каждого** контрола ≥44×44 CSS px». Кнопки ↑/↓/удалить у
+блоков и строк в `summary-panel-editor.ts` используют этот класс и физически
+дают 34×34 px — на 23% меньше контракта. Добавленный в этом же диффе
+`docs/TOUCH-SUPPORT.md` (строки 48-51) сам сузил обещание до «панель и обе
+половины составного контрола» + «диалог сохраняет доступность контента» —
+то есть документация уже разошлась с числовым контрактом принятого ТЗ,
+не будучи для этого отдельно согласована с владельцем (§7.1 разрешает менять
+только продуктовые решения владельца, не через документацию по умолчанию).
+
+**Чем краснеет:** проверено чтением CSS и вычислением фактического размера
+(34 vs заявленных 44); отдельного автотеста на размер контролов в диффе нет
+(не искал, чтобы не плодить ложный вывод — `grep -n "44"` по тестам/смокам
+не нашёл проверок размера ни для одного контрола формы).
+
+**Правка:** увеличить `.summary-editor-row button` (и любые другие
+интерактивные элементы формы у которых итоговый tap-target <44px) минимум до
+44×44 либо явно обновить и ТЗ, и `TOUCH-SUPPORT.md` с owner-видимой правкой
+контракта, если 34px — осознанное сужение только для мыши/клавиатуры (тогда
+формулировка ТЗ «каждого контрола» вводит в заблуждение и тоже требует
+правки).
+
+### M4 — идентичность карточки (AC21) не имеет ни выделенного резолвера, ни свидетеля теста
+
+**Файл:** `src/summary-panel-runtime-loaded.ts:236-250` (`placementSlot`).
+
+ТЗ §8.2 требует: «Изолированный resolver связывает enclosing `hui-card` с
+логическим индексом в native view: Sections — `[viewIndex, sectionIndex,
+cardIndex]`, Masonry — индекс в исходном упорядоченном `cards`, **не номер
+визуальной колонки**». В коде единственный механизм — `placementSlot()`,
+общий для всех случаев: подъём по `parentNode`/shadow-host с записью
+`localName:childIndex` на каждом уровне. Для Masonry-вида HA перераспределяет
+карточки по колонкам по высоте при изменении ширины окна — то есть DOM-индекс
+карточки в родителе именно то, что ТЗ называет «номером визуальной колонки» и
+явно запрещает использовать. AC21 обещает доказательство «Native
+Sections/Masonry/nested fixtures, reload/remount/reflow» — этого нет ни в
+`test/summary-panel.test.mjs` (нет упоминаний `placementSlot`/`preferenceKey`/
+identity), ни в `demo/smoke_summary_panel.mjs` (по описанию хендоффа: «right/
+bottom, small-card hide/restore, local intent, overlay geometry, read-only
+surface, lazy full form» — без identity/masonry/reflow).
+
+Признаю: §14 ТЗ помечает механику «per-card key по logical host path»
+техническим решением, свободным для правки ревьюером — значит сам факт
+отказа от буквального `[viewIndex, sectionIndex, cardIndex]` не обязан быть
+находкой сам по себе. Находка именно в том, что **защитный AC без названного
+свидетеля** (PROCESS §2.7) — нет ни одного теста, который доказывал бы, что
+выбранная общая реализация действительно переживает Masonry-реflow, при том
+что это ровно тот сценарий, который ТЗ явно называет риском.
+
+**Чем краснеет:** не воспроизведено исполнением (реальный `hui-masonry-view`
+недоступен в demo-стенде — честно пишу «проверено чтением, не
+исполнением»); риск обоснован конкретным механизмом браузерного
+column-балансирования Masonry-вида, который переставляет карточки между
+колонками при изменении ширины без изменения состава `cards` в конфиге.
+
+**Правка:** либо добавить smoke/unit фикстуру, эмулирующую изменение
+DOM-порядка карточки без изменения logical config (переставить `children` у
+фейкового контейнера и убедиться, что `preferenceKey()`/`placementSlot()` не
+меняется), либо прочитать логический индекс из `hui-view`/`lovelace` config,
+если он доступен, вместо позиции в DOM.
+
+## Проверено и корректно
+
+- **Модель/валидация** (`summary-panel.ts`): `cpLength` считает Unicode
+  code points тем же способом, что backend `len(str)` (Python 3 строки —
+  всегда code points) — лимиты 48/64 согласованы на обоих концах, подтверждено
+  тестами по обе стороны и мутацией backend-регекса (см. ниже).
+- **Backend reference validation** (`validate_summary_panel_references`):
+  мутация (убрал проверку `entity_id not in readable_entity_ids`) уронила
+  `test_reference_validation_allows_old_broken_ids_but_rejects_new_ones` —
+  тест умеет падать, защита реальна.
+- **Совместимость** (`preserve_summary_panel_namespace`,
+  `_future_summary_panel`): старый клиент не стирает namespace, будущая
+  неизвестная версия остаётся lossless — оба пути покрыты
+  `test_old_client_omission_preserves_namespace_and_future_version_losslessly`.
+- **Приватность** (`test_support_package.py`): добавленный `summary_panel`
+  с приватными строками явно исключён из `plan_backup` (`assert
+  "summary_panel" not in package[...]`), тест прогнан и зелёный.
+- **Ленивая загрузка**: `test/summary-panel.test.mjs` проверяет, что
+  `summary-panel-editor`/`-runtime-loaded` не попадают в `initialViewFiles`
+  манифеста и что карточка не импортирует редактор статически — подтверждено
+  чтением манифеста и самим прогоном.
+- **Отсутствие сервисных вызовов / только текст**: регэксп-тест на
+  `callService(` в загруженном рантайме и редакторе зелёный; весь
+  пользовательский текст идёт через `lit-html` интерполяцию (авто-экранирование),
+  `unsafeHTML` нигде не импортирован — проверено чтением обоих файлов.
+  Совпадает с secure-device инвариантом SCOPE.md (замки эта задача не трогает).
+- **aria-pressed** отражает `local.show` (сохранённое намерение), а не
+  временную видимость от mobile/fits — проверено чтением `renderControls`,
+  соответствует явному требованию §4.1 ТЗ.
+- **Layout-математика** (`resolveSummaryLayout`): юнит-тест воспроизводит
+  ровно граничные примеры из ТЗ §5.2 (303/304, 185/186, 245/246 с
+  `controlTop:72`) — совпадает побитово.
+- **Разделение shared/local Save**: `saveDialog` не трогает `localShow` при
+  ошибке записи и не откатывает уже применённый local при отказе сервера —
+  прочитано, соответствует §9.1 контракту «атомарно значит без частичного
+  draft», конфликт (`_cfgRev !== baseRevision`) корректно триггерит `conflict`.
+  Не проверено исполнением two-client сценария (см. ниже).
+- **Таймер часов**: единственный `setTimeout` на видимую панель с датой,
+  снимается на `hidden`/disconnect, пересчитывает контекст (locale/tz) —
+  прочитано, логика соответствует §7.4 («не создавать timer на строку»).
+- **device_count/area кэш**: мемоизация по `cfgEpoch`/`layoutRev`/
+  `registryRev` — обычное обновление состояния сущности не должно
+  перезапускать union/roster; прочитано, инвалидация ключей выглядит верной,
+  не проверено performance-профилем (см. «не проверял»).
+
+## Чего не проверял и почему
+
+- **Полный HA backend harness** (`test_ha_*.py`) — окружение без
+  `.venv-backend`; доверяю отчёту автора без независимой перепроверки.
+- **Golden/скриншоты** — предрелизный гейт по PROCESS §8, diff не меняет
+  существующий рендер плана (overlay вне SVG-слоя, подтверждено чтением).
+- **Полный набор `demo/smoke_*.mjs`** — не оправдано объёмом диффа: прогнал
+  выборочно новый смок и единственный существующий смок, реально задетый
+  изменённым кодом (`smoke_kiosk.mjs`, см. M1); остальные 230 не выбраны
+  диффом по существу (шум smoke-select — общие имена полей).
+- **Performance-профили** (AC25/§12) — не названы блокирующими для этого
+  раунда; логика памятки/таймеров проверена чтением, не измерением.
+- **Реальное Masonry/Sections-переразмещение HA** — недоступно в
+  demo-стенде; см. M4, отмечено как «проверено чтением».
+- **Многоклиентная конкурентная запись (AC19)** — проверено чтением кода
+  optimistic-locking пути (`expected_rev`, `_cfgRev`), не воспроизведено
+  реальным two-client прогоном.
+- **Реальные touch-устройства** — размер tap-target оценён по CSS-значениям
+  (см. M3), не по замеру на физическом экране.
+
+## Итог
+
+High: 0 · Medium: 4 (все в скоупе, чинятся в этой же задаче) · Low: 0.
+Три из четырёх находок (M1, M2, M3) — конкретные, воспроизведённые
+инструментально в этой сессии (упавший смок, две мутации без реакции теста,
+измеренный CSS-размер). M4 — разобрана чтением с честной пометкой, куда
+дотянуться исполнением не удалось.
+
+Вердикт: жёлтый. Возврат автору на исправление M1–M4 в рамках текущей
+задачи, без нового issue.
+
+---
+
+<!-- material-anchors: сгенерировано конвейером (#414) -->
+
+## Материал раунда
+
+- Ветка: `issue/437-summary-panel`, коммит `96e07b9a0255` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
+- Дерево материала: `ae3f49aa4859ba86376dd881a1dea9e7d1e08ba2`
+  ```
+  git log --all --format='%H %T' | grep ae3f49aa4859
+  ```
+- ТЗ `docs/specs/437-summary-panel.md`, блоб `58a2db80c2161079bc9044c107063e54bfcf0be6`
+  ```
+  git log --all --find-object=58a2db80c2161079bc9044c107063e54bfcf0be6 -- docs/specs/437-summary-panel.md
+  ```