From 361ec47856cc24897a3de48c3e051752f412cbee Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 27 Aug 2026 23:00:48 +0000 Subject: [PATCH] docs: review document for #29 Issue: #29 User-Visible: no --- docs/reviews/CODE-REVIEW-29-r1.md | 281 ++++++++++++++++++++++++++++++ 1 file changed, 281 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-29-r1.md diff --git a/docs/reviews/CODE-REVIEW-29-r1.md b/docs/reviews/CODE-REVIEW-29-r1.md new file mode 100644 index 00000000..2f8f43c2 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-29-r1.md @@ -0,0 +1,281 @@ +# CODE-REVIEW-29-r1 + +- Issue: #29 «[HP-UX-02] inbox и жизненный цикл устройств» +- Этап: code (PROCESS.md §2.7) +- Заход: r1 · блокирующих циклов израсходовано 0 из 4 (первый реальный код-ревью: + предыдущая попытка была остановлена до чтения кода из-за конфликта ребейза — + см. комментарий issue от 2026-08-27T22:42:46Z — поэтому дельта-режим §2.9 не + применяется, разбор ниже полный) +- Проверено на SHA `905d4847a3797a8830eb6c5c509351f142193b73` (ветка + `issue/29-device-inbox-lifecycle`, перебазирована на `origin/dev@2c20f2d`) +- ТЗ: `docs/specs/029-device-inbox-lifecycle.md`, зелёный spec-review r2 + (`docs/reviews/SPEC-REVIEW-29-r2.md`) + +## Скоуп изменения + +Один коммит `905d4847` (Issue #29, User-Visible: yes, оба CHANGELOG в этом же +коммите): + +- новый чистый модуль `src/device-inbox.ts` (281 строка): `bindingCandidates` + (общий eligibility-helper, вынесенный из `_bindingCandidates`), + `buildDeviceInbox` (резолвер строк каталога), `filterDeviceInbox`; +- `src/houseplan-card.ts`: диалог каталога устройств (`_renderDeviceInbox` и + ~15 приватных методов), замена кнопок «Добавить»/«Скрытые и деактивированные» + на «Устройства», перенос ghost-toggle внутрь каталога; +- `src/styles/dialogs.styles.ts` (+135), `src/hp-dialog.ts` (параметризована + ширина wide-диалога); +- `src/i18n/en.json` / `ru.json` (+45 ключей `device_inbox.*`, обновлён + `marker.hide_tip`); +- `test/device-inbox.test.mjs` (154 строки, чистый модуль); +- `demo/smoke_device_inbox.mjs` (новый), `demo/smoke_hidden_flag.mjs` + (адаптирован под перенос ghost-toggle); +- `demo/golden/{harness,matrix}.mjs` — 3 новых golden-сценария + (`device-inbox-desktop-en-light/ru-dark/narrow-ru-dark`); +- `scripts/mutation-gate.mjs`, `tsconfig.test.json` — точечные правки под + перенос кода в `device-inbox.ts`; +- документация: `docs/ARCHITECTURE.md`, `docs/FILTERING.md`, `docs/TESTING.md`, + `docs/USER-GUIDE.md`/`.ru.md`, `docs/STATUS.md`, `docs/CHANGELOG.md`/`.ru.md`, + golden-скриншоты и `screenshots.json` (docs-check зелёный, см. ниже). + +## Как проверялось + +Гейты, реально прогнанные мной на дереве SHA `905d4847`: + +| Команда | Результат | +|---|---| +| `npx tsc --noEmit` | зелёный, без вывода | +| `npm test` | `# tests 1400 / pass 1399 / fail 0 / skipped 1` | +| `npm run build` | собран `dist/houseplan-card.js` за 10.8s | +| `md5sum dist/… custom_components/…` | идентичны после чистой пересборки (bundle:sync подтверждён) | +| `node scripts/check-docs.mjs` | `Documentation checks passed (7 files, 10 external links)` | +| `npm run bundle:sync` | пересобрал и синхронизировал все три копии, включая недостающую `demo/srv/assets/…` | +| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | 75 прямых совпадений, много «зарегистрированных связей», 41 слабая связь (полный вывод ниже) | +| `node demo/smoke_device_inbox.mjs` | `OK`, все 12 проверок true | +| `node demo/smoke_binding_picker.mjs` | `OK`, все 24 проверки true | +| `node demo/smoke_hidden_flag.mjs` | `OK`, все 27 проверок true | +| `node scripts/mutation-gate.mjs --id=device-tombstone-blocks-child-picker` | `поймано 1 из 1` — тест умеет падать | +| `npm run inventory` | Node unit 1400, backend pure 198, HA-harness 143, browser smokes 194 (совпадает с числами автора после ребейза) | +| `node demo/smoke_editor_tabs.mjs` | **FAILED** — `devBarBtns: expected true, got false` (находка H1 ниже) | +| `gh run view` на точный SHA `905d4847` | CI уже прогнан и **red**: job «Смоки в браузере (шард 2 из 3)» упал именно на `smoke_editor_tabs` с той же ошибкой; job «Golden» упал на 3 `missing-baseline` для новых сценариев (ожидаемо, см. «Что не проверялось») | + +`smoke-select` — «прямое совпадение» (75) в основном ложноположительное: почти +все совпадения идут по родовым символам `_markerDialog`/`_openMarkerDialog`/ +`_maybeRebuildDevices`/`_infoCard`/`_config`/`_spaceDialog`/`_zoom`, которые +задеты только потому, что `this._markerDialog = null` заменено на +`this._closeMarkerDialog()` в нескольких местах кода, не относящихся к +каталогу. Прочитал `_closeMarkerDialog()` (houseplan-card.ts:14173-14186): вне +пути возврата из каталога (`_deviceInboxReturn` не установлен) он делает ровно +то же самое, `this._markerDialog = null`, — поведение для всех смоков вне темы +каталога не меняется. Прогонять все 75 не стал; прогнал три прямо тематических +(`smoke_device_inbox`, `smoke_binding_picker`, `smoke_hidden_flag` — все зелёные +выше) и, поскольку инструмент относит `smoke_editor_tabs.mjs` только к «слабой +связи» (общее имя `_saveConfig`), посмотрел его отдельно из-за прецедента +#234 («по названию не про тему — на деле регрессия») — и он оказался красным +(находка H1). Остальные слабые связи не прогонял: все они делят по одному +общему символу (`_saveConfig`/`_applyView`/`_zoom`/`_model` и т.п.) с +изменёнными строками, не относящимися по смыслу к каталогу устройств. + +Прочитан полный diff `src/houseplan-card.ts` (687 строк) построчно и полностью +модуль `src/device-inbox.ts`; сверено с ТЗ по каждому AC1–AC11 (детали в +следующем разделе), с `docs/SCOPE.md` (J4/J6, admin persona, desktop-first) и +с `docs/USER-GUIDE.ru.md` (терминология «На плане/Доступны/Скрытые/Доступны +снова», «Показывать скрытые на плане» и т.д. совпадает 1:1 с UI-строками). + +## Находки + +### H1 — `demo/smoke_editor_tabs.mjs` сломан этим диффом (High, в скоупе) + +`demo/smoke_editor_tabs.mjs:159` — существующая, не тронутая этим PR строка: + +```js +out.devBarBtns = sr().querySelectorAll('.editbar.devbar .btn:not(.barclose)').length === 3; // add/show-all/rules (v1.33.2: Reset removed) +``` + +ТЗ §10.1 сознательно заменяет две кнопки («Добавить», «Скрытые и +деактивированные») на одну («Устройства»), оставляя «Правила иконок» — +итоговый devbar имеет 2 кнопки, не 3. Ассерт не обновлён. + +Воспроизведение — не гипотеза, дважды подтверждено: + +1. Локально: `node demo/smoke_editor_tabs.mjs` → `FAILED (1): - devBarBtns: + expected true, got false`. +2. В CI на точном SHA задачи: run `33123777345` (`Проверка (CI)`, SHA + `905d4847…`), job «Смоки в браузере (шард 2 из 3)» + (https://github.com/Matysh/houseplan-card/actions/runs/33123777345/job/98697171028) + уже красный с той же строкой: `FAIL smoke_editor_tabs` / + `devBarBtns: expected true, got false`. + +Это ровно тот класс дефекта, о котором предупреждает прецедент #234: смок +называется «editor_tabs» и не намекает на устройства, `smoke-select` относит +его только к слабой связи по общему имени `_saveConfig` — но по факту это +прямая регрессия темы задачи. `smoke` — один из обязательных gate-джобов +(`AGENTS.md`: `docs, provenance, process-gate, hacs, hassfest, frontend, +smoke, golden, performance_smoke, backend`), и он объективно красный на SHA +задачи прямо сейчас, а не гипотетически. Фикс тривиален и целиком в скоупе +задачи: обновить ожидаемое число (и комментарий `add/show-all/rules`) до 2 +кнопок/`devices/rules`; отдельный issue не заводится (решение владельца +2026-08-19, #202). + +### M2 — «комната» в каталоге — это имя HA-зоны, а не название комнаты плана (Medium, в скоупе) + +`src/houseplan-card.ts`, `_deviceInboxRows()`: + +```js +const areaNames: Record = {}; +for (const [id, area] of Object.entries(this.hass?.areas || {})) areaNames[id] = area?.name || id; +``` + +Это единственный источник поля `areaName`/поисковой строки для «комнаты» в +каталоге (`src/device-inbox.ts` строит `searchText` и мета-строку строго из +переданных `areaNames`). Значение берётся из **имени HA-зоны** (`hass.areas`), +а не из **имени комнаты плана** (`RoomCfg.name`), которое пользователь +свободно переименовывает независимо от HA (единственное место записи — +`room.name = this._nameSel.trim() || room.name` в диалоге комнаты, +houseplan-card.ts:19386, без всякой синхронизации с зоной HA) и которое +показывается везде на самом плане (подпись комнаты на канвасе — +`${r.name}` — houseplan-card.ts:19054; sub-label бинд-пикера — +`(sp.title || sp.id) + ' · ' + r.name`, houseplan-card.ts:14571/14576). + +Сам каталог явно различает эти два понятия: причина `no_bound_room` +переведена как «Зона HA не связана с комнатой плана» — то есть «зона» и +«комната» осознанно не одно и то же. При этом ТЗ §10.2 требует показывать в +строке «пространство и комнату», а `docs/USER-GUIDE.ru.md` (этот же коммит) +прямо обещает: «Поиск работает по имени, модели, интеграции, комнате, +пространству и exact binding» — то есть комнатой пользователю обещан +объект, названный так, как он называется на плане, а не как называется зона +Home Assistant. + +Воспроизводимый сценарий: пользователь переименовал в редакторе плана +комнату «Kitchen» (имя зоны HA) в «Кухня-столовая» (`room.name`). На плане и +в бинд-пикере отображается «Кухня-столовая». В каталоге устройств та же +строка покажет и будет искаться по «Kitchen» — расхождение с тем, что видно +на самом плане, прямо в задаче, продукт которой — «объяснить, почему +устройство там, где оно есть» (§2 «После» ТЗ). Это не гипотетический边ge +case: `room.name` независимо редактируется в стандартном рабочем процессе, не +только в экзотике. + +Исправление в скоупе: строить `areaNames` (или отдельный `roomNames`) из +`this._areaToSpace[areaId]?.room?.name`, а не из `this.hass?.areas`; на +`hass.areas` можно откатываться только для `available`-кандидатов без +привязанной комнаты (`no_bound_room`), где комнаты действительно ещё нет. + +### L3 — `DeviceInboxRow.canOpenHa` — мёртвое поле (Low) + +`src/device-inbox.ts:245`: `const canOpenHa = kind === 'device' || kind === +'entity';` — тривиально всегда `true` (у `kind` только два значения), и само +поле нигде не читается: рендер каталога (`houseplan-card.ts`) использует +отдельный, независимый метод `this._bindingHasHaPage(row.binding)` для того +же решения. Не влияет на поведение — предлагаю удалить поле как неиспользуемое, +либо, если оно задумано для другого потребителя, использовать per отчёту. +Не блокирует; можно оставить с пометкой в этом документе (снято ревьюером). + +## Проверено чтением и признано корректным + +- **AC1** (единая точка входа): подтверждено кодом и `smoke_device_inbox` + (`oneCatalogEntryPoint`), плюс `smoke_editor_tabs` фактически доказывает + замену кнопок (см. H1 — сам факт правильный, сломан только счётчик). +- **AC2** (детерминированная классификация): приоритет `removed → hidden → + on_plan → available` в `buildDeviceInbox` (device-inbox.ts:219-224) читается + ровно как в §7.2 ТЗ; unit-тест «full lifecycle matrix» покрывает auto + no-marker, manual/automatic hidden, ha_disabled, orphaned, unverified, + removed active/missing, parent-tombstone+live-child, synthetic light group, + candidate без комнаты — все проходят зелёным. +- **AC3** (auto/new): `isNew: !!runtime && newDeviceIds.has(runtime.id)`, + подтверждён unit-тестами (`device:auto`/`device:d3`) и не снимается Find + (`_findInboxDevice` не трогает `_newIds`), снимается только через + `_openMarkerDialog` → `_ackNewDevice` (существующий контракт, не менялся). +- **AC4** (lifecycle/HA-status независимы): проверено unit-тестом + «HA status overlays lifecycle…» — `hidden:true+ha_disabled` остаётся в + `hidden`, `canShow=false`, `canFind` включается только вместе с + `showHiddenOnPlan`; `limited registry` (`unverified`) не создаёт ложный + disabled/orphaned — статус остаётся `unverified` дословно. +- **AC5** (exact binding/re-add): device-parent tombstone и live entity child + дают разные строки (`readd`/`on_plan`) в unit-тесте №1; логика материализации + Re-add переиспользует немодифицированный `_saveMarker`/`deletePlanMarkerRecords` + (контракт #262), только точка входа `_closeMarkerDialog` заменяет + `this._markerDialog = null`. Подтверждено `smoke_binding_picker` (24/24) и + mutation-gate (`device-tombstone-blocks-child-picker` — поймано 1/1). +- **AC6** (действия): Find/Edit/Hide/Show/Add/Hide-from-list/Re-add сверены + построчно с таблицей §10.3; `_setInboxHidden` использует существующий + `marker.hidden`, не создаёт tombstone, требует `status.kind==='active'`, + что совпадает с `canHide`/`canShow` в резолвере. Ghost-toggle перенесён и + проверен `smoke_hidden_flag` (`ghostToggleMovedIntoCatalog`, `ghostInEditor` + и весь остальной набор — 27/27). +- **AC7** (read-only): открытие/поиск/смена вкладки/Show more/Find не + вызывают `_saveConfig`/websocket-запись — проверено чтением всех + обработчиков (только присвоения `this._deviceInbox = {...}`) и + подтверждено `smoke_device_inbox.browsingIsReadOnly` (перехват `callWS`, + сравнение `config/layout/cfgRev/layoutRev` до/после — 0 записей). +- **AC8** (возврат/refresh): `_deviceInboxReturn` восстанавливает + tab/search/filter и `anchor` после Cancel/Save, `_deviceInboxMemo` + инвалидируется по `cfgRev/cfgEpoch/regSignature/newSyncKey` — подтверждено + `smoke_device_inbox.nestedCancelReturnsContext`. +- **AC9** (поиск/большие реестры): `bindingCandidates` не режет список до + фильтрации (только глобальная сортировка), пагинация (`limit`) применяется + строго после `filterDeviceInbox` — подтверждено unit-тестом на 260 сущностях + (находит `entity_259`, за пределами старого cap=200) и + `smoke_device_inbox.searchUsesFullSnapshot`. +- **AC10** (accessibility/responsive): `role=tablist`/`aria-selected`, + `aria-live=polite`, стрелочная навигация (`_deviceInboxTabKey`, + подтверждена `smoke_device_inbox.arrowChangesTab`), нет горизонтального + скролла на десктопе (`noHorizontalOverflow`) и в golden narrow-сценарии + (`device-inbox-narrow-ru-dark`, ширина 390px, harness проверяет + `scrollWidth <= clientWidth+1`). Полный ручной keyboard-проход (Tab по всем + действиям, focus-restore при закрытии) не выполнялся — см. «Что не + проверялось». +- **AC11** (compatibility): `npm test` зелёный целиком (1399/1400, 1 + пропущен), включая незатронутые regression-наборы `devices.test.mjs`, + `ha-binding-status.test.mjs`; miграция конфигурации не добавлена + (`filter_seeded`/`show_all` логика не изменена по существу — см. отдельно + ниже). +- Три golden-сценария (`device-inbox-desktop-en-light/ru-dark`, + `device-inbox-narrow-ru-dark`) добавлены в матрицу и harness-препарация + проверяет 4 вкладки + отсутствие горизонтального переполнения. +- Трейлеры коммита корректны: один коммит, `Issue: #29`, `User-Visible: yes`, + оба CHANGELOG в этом же коммите. +- Отдельно проверил переход `_showAll`/`_showHidden`/legacy `settings.show_all` + (комментарий автора в коде: «Legacy configs still honour it through + buildDevices until they are seeded»): для конфигов до `filter_seeded` окно, + где чекбокс «Показывать скрытые на плане» не мог бы выключить унаследованный + `show_all:true`, закрывается автоматически при первом + `_maybeRebuildDevices()` редактирующего клиента (сидер удаляет + `settings.show_all`, houseplan-card.ts:3693) — это существующий, + немодифицированный этой задачей механизм; не завожу как находку. + +## Чего не проверял и почему + +- **Полный набор `demo/smoke_*.mjs` (194 файла)** — не прогонял целиком; + прогнал 4 тематических (`device_inbox`, `binding_picker`, `hidden_flag`, + `editor_tabs` — последний отдельно из-за прецедента #234) плюс сверил + вывод `smoke-select`. Полный набор — обязанность pre-release гейта + (PROCESS.md §8) и одновременно уже прогнан в CI (см. H1) — красный сейчас + именно на `editor_tabs`, других красных смоков в этом прогоне CI не было. +- **`npm run golden:verify` / приёмка baseline** — не прогонял. Три новых + golden-ID (`device-inbox-*`) не имеют базовых кадров; CI на этом SHA уже + показал `missing-baseline` для всех трёх (job «Golden», run 33123777345) — + это ожидаемое, задокументированное поведение (PROCESS.md: baseline + принимается только `npm run golden:accept -- --reviewed` на полном Linux + CI-артефакте, не в рамках код-ревью), а не находка. +- **`python -m pytest tests_backend`** — diff не трогает + `custom_components/**/*.py`; backend-код не менялся. +- **Performance-профили** — АС/риски (§17 ТЗ) не called out perf budget, + `smoke-select` не выделил performance-чувствительные пути; unit-тест на + 260 записях покрывает синтетический large-registry сценарий, отдельный + benchmark не запускал. +- **Ручной keyboard/screen-reader проход** — полагался на чтение разметки + (roles/aria-атрибуты) и golden/smoke автоматику; полного ручного прохода + Tab-порядка по всем действиям строки не делал. +- **71 из 75 «прямых совпадений» `smoke-select`** — не прогонял; обоснование + см. в разделе «Как проверялось» (общие символы `_markerDialog` и т.п., + поведение вне пути возврата из каталога не меняется — проверено чтением + `_closeMarkerDialog`). + +## Вывод + +Один подтверждённый High (сломанный существующий смок, воспроизведён и +локально, и в реальном CI-прогоне на SHA задачи) и один Medium в скоупе +(источник имени «комната» в каталоге — HA-зона вместо имени комнаты плана, +расходится с остальным UI и с обещанием user-guide). Оба чинятся в этой же +задаче без нового issue. Функциональное ядро (резолвер, read-only контракт, +поиск, re-add/exact-binding семантика) проверено тестами, которые умеют +падать (mutation-gate), и не вызывает возражений.