From fe18f80fc272bdc91c6d97d59eef5c07db1f158f Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 3 Sep 2026 16:47:41 +0000 Subject: [PATCH] docs: review document for #162 Issue: #162 User-Visible: no --- docs/reviews/CODE-REVIEW-162-r1.md | 314 +++++++++++++++++++++++++++++ 1 file changed, 314 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-162-r1.md diff --git a/docs/reviews/CODE-REVIEW-162-r1.md b/docs/reviews/CODE-REVIEW-162-r1.md new file mode 100644 index 00000000..f11d3ca4 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-162-r1.md @@ -0,0 +1,314 @@ +# Код-ревью #162 — заход r1 + +- Issue: [#162](https://github.com/Matysh/houseplan-card/issues/162) +- Этап: code (PROCESS.md §2.7) +- Ветка: `issue/162-vacuum-map-space-routing`, HEAD `4032b810` +- ТЗ: `docs/specs/162-vacuum-map-space-routing.md`, утверждено на редакции 2 / spec-review r2, SHA `4c56b389` +- Заход: r1 (первый код-ревью для этого issue) · блокирующих циклов израсходовано 0 из 4 + +## Вердикт + +**Красный.** High: 1, Medium: 1 (оба в скоупе задачи, чинятся в ней же). + +## Скоуп проверки + +Материал: `git log --oneline origin/dev..HEAD` (11 коммитов) и +`git diff origin/dev...HEAD` (71 файл). Ревью полное (не дельта): это первый +код-ревью цикл для #162. + +Прочитано и сверено построчно: + +- `src/vacuum-routes.ts` и питоновское зеркало `custom_components/houseplan/vacuum_routes.py` — + типы, `validateMarkerRoutes`/`validate_marker_routes`, `effectiveRoutes`/`effective_routes`, + `resolveRoute`/`resolve_route`, `adoptLegacyRun`/`adopt_legacy_run`, `planVacuumOverlay`, + `routeWarningKey`; +- `src/vacuum-route-edit.ts` — `addRoute`, `removeRoute`, `changeRouteSpace`, + `saveRouteCalibration`, `convertLegacyRoutes`, `writeVacuumMatrix`, `planVacuumFit`, + `calibrationTarget`; +- `src/houseplan-card.ts` — снимок фактов (`facts.set('vacuum:', …)`), новая + `_renderVacuums` (рендер по `route.space`, а не по фильтру устройств дока), + `_vacRouteBadge` (amber warning); +- `src/houseplan-editor-runtime.ts` — `_vacSaveMatrix`, `_vacAutoCalibrate`, + `_vacApplyCalibrationProposal`, `_vacStartFit`, `_vacFitSave` (весь путь калибровки); +- `src/editors/vacuum-maps-section.ts` — весь UI блока «Карты и этажи»; +- `src/space-deletion.ts` — `routesIntoSpace`, `collectSpaceMarkerDependencies`, + `createSpaceDeletionCandidate` (AC16); +- `custom_components/houseplan/trails.py` — `same_run_identity`, + `can_resume_trail_run`, `TrailBook.on_point`/`drop_unknown_routes`, + `TrailRecorder.async_refresh`/`async_purge_orphans`/`_route_id`; +- `custom_components/houseplan/validation.py` — `validate_marker_vacuum_routes` + (change-aware, как badge-валидатор) и её проводка в + `websocket_api.py` (config/set, optimize) и `import_export.py` (оба import flow); +- `scripts/mutation-gate.mjs` — определения восьми заявленных + новых мутантов + (14 по факту), два прогнаны лично (см. ниже); +- `docs/CHANGELOG.md`/`.ru.md`, `docs/VACUUM.md`, `docs/USER-GUIDE.ru.md`, + `docs/CONFIG-COMPATIBILITY.md`, `docs/ARCHITECTURE.md`, `docs/TESTING.md`. + +Не читал построчно (низкий риск, не в фокусе AC): i18n-словари полностью +(точечно сверил RU/EN парность нужных ключей), `docs/images/screenshots.json` +(бинарный/сгенерированный артефакт), `dist/**` и +`custom_components/houseplan/frontend/**` (класс D, генерируются билдом — +сверил, что `bundle:sync` не оставляет расхождений). + +## Гейты — что прогнал сам и с каким результатом + +Зелёного Validate на SHA `4032b810` не найдено, поэтому прогнал сам: + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | чисто | +| Unit (frontend) | `npm test` | 1854 pass / 0 fail / 1 skip | +| Build | `npm run build` | собирается, `dist` создан | +| Docs fingerprint | `node scripts/check-docs.mjs` | `Documentation checks passed (7 files, 10 external links)` | +| Backend (пул #162) | `python3 -m pytest tests_backend/test_trails.py tests_backend/test_trail_recorder.py tests_backend/test_vacuum_route_validation.py tests_backend/test_vacuum_routes.py -q` | **70 passed** | +| Backend (полный доступный набор) | `python3 -m pytest tests_backend -q --ignore=test_ha_setup.py --ignore=test_ha_upload.py --ignore=test_ha_virtual_lights.py --ignore=test_ha_websocket.py --ignore=test_ha_import_export.py` | 363 passed, 8 failed — все 8 падений в `test_coordinate_canonicalization.py`/`test_ha_support_transport.py` из-за отсутствующего `pytest-asyncio` в этой песочнице (окружение, не #162; `test_ha_*` файлы у меня тоже не поднимаются — нет `pytest_homeassistant_custom_component`, canonical harness тут недоступен, как и задокументировано в AGENTS.md) | +| Mutation gate (точечно) | `node scripts/mutation-gate.mjs --id=vacuum-calibration-solves-against-the-dock` | `поймано 1 из 1` | +| Mutation gate (точечно) | `node scripts/mutation-gate.mjs --id=vacuum-overlay-back-to-the-dock-space-filter` | `поймано 1 из 1` | +| Смок (профильный) | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | 24 прямых совпадения (список ниже), 37 слабых связей | +| Смок `smoke_vacuum_multifloor.mjs` | запускался автором, вывод сверен по коду — логика соответствует | не перезапускал: сценарий не пересекается с найденным дефектом (см. Находка 1) | + +Смоки из «прямого совпадения», которые не гонял отдельно (автор уже прогнал +пересекающийся набор — `smoke_vacuum`, `smoke_vacuum_firstuse`, +`smoke_cold_view_vacuum`, `smoke_danger_confirm_branches`; остальные 20 — +`_confirmDanger`/`_config`/`_model`/`_saveConfig`/`DevItem` только по общей +инфраструктуре редактора, не по vacuum-логике — не пересекаются с изменённым +кодом по существу): не прогонял, полный набор здесь избыточен относительно +diff. `smoke_editor_gestures.mjs` (совпадение по `_vacFit`) не гонял — вместо +него написал точечный воспроизводящий скрипт (Находка 1), который прямо +отвечает на вопрос, для которого этот смок мог бы пригодиться. + +Golden и performance не гонял: diff не меняет визуальный стиль puck/path +(§10.1 ТЗ явно это запрещает) и не затрагивает performance-чувствительные +пути сверх заявленного в АC20/пре-бета цикла; они канонически прогоняются +перед бетой (AGENTS.md «Gates»), не на этом ревью. + +## Находка 1 (High, в скоупе) — калибровка мимо своего этажа в пути «высокий residual → ручная подгонка» + +**Файл:** `src/houseplan-editor-runtime.ts:11043-11062`, метод `_vacApplyCalibrationProposal`. + +```ts +public _vacApplyCalibrationProposal(manual: boolean): void { + const proposal = this.host._vacCalConfirm; + ... + if (manual) { + const dev = this.host._devices.find((candidate) => candidate.id === proposal.markerId); + const fit = fitFromMatrix(proposal.matrix); + if (!dev || !fit) return; + this.host._markerDialog = null; + if (dev.space !== this.host._space && !this.host._commitSpace(dev.space)) return; + this.host._vacFit = { + markerId: proposal.markerId, source: proposal.source, routeId: proposal.routeId, + mapId: proposal.mapId, p: fit, drag: null, + }; + return; + } +``` + +`_vacAutoCalibrate` (той же файл, стр. 11020-11042) правильно вычисляет +`target = calibrationTarget(d.id, d.marker?.vacuum, d.space, src, mapId)` и +решает матрицу против `target.space` (это и есть коммит `4032b810`, AC8). +Когда residual выше порога, он кладёт в `_vacCalConfirm` только +`{markerId, source, mapId, routeId: target.routeId, matrix, rooms, error}` — +**без `space`**. Когда пользователь выбирает «Подогнать вручную» в диалоге +подтверждения, `_vacApplyCalibrationProposal(true)` заново берёт `dev.space` +(пространство ДОКА), а не пространство маршрута, которое было использовано +для решения матрицы. Для сравнения, независимый путь `_vacStartFit` +(стр. 11073-11083) делает это правильно: коммитит `plan.space`, посчитанный +`planVacuumFit`. + +**Воспроизведено исполнением** (не только чтением): сборка + demo harness, +сценарий — робот с доком на `f1` и route `vr2` (`m2`) на `garden`, `_space` +уже равен `f1` (типичная ситуация: диалог устройства открыт с этажа дока). +Симулирован ровно тот `_vacCalConfirm`, который кладёт `_vacAutoCalibrate` при +высоком residual для `vr2`, затем вызван `_vacApplyCalibrationProposal(true)`: + +```json +{ + "spaceBefore": "f1", + "spaceAfter": "f1", + "fitOpened": true, + "fitRouteId": "vr2", + "expectedRouteSpace": "garden" +} +``` + +`_space` остаётся `f1`, хотя `fitRouteId` — `vr2`, чья `route.space` — +`garden`. Диалог ручной подгонки открывается на этаже ДОКА и рисует матрицу, +решённую против комнат `garden`, поверх геометрии `f1`: это ровно тот +дефект, ради исправления которого создан #162 (робот показывается не на том +этаже — только внутри редактора калибровки, а не в View), и прямое +нарушение AC8 («Auto/manual calibration использует rooms/viewBox/cell size +target `route.space`, а не dock space»). + +**Почему не поймано мутант-гейтом.** Заявленный мутант +`vacuum-calibration-solves-against-the-dock` (проверил лично — +`node scripts/mutation-gate.mjs --id=vacuum-calibration-solves-against-the-dock` +→ «поймано 1 из 1») патчит саму функцию `calibrationTarget()` и охраняется +unit-тестом на неё же. Он честно защищает то, что защищает, но не покрывает +этот конкретный стейтфул-вызов в `_vacApplyCalibrationProposal`, который +пересчитывает `dev.space` заново вместо того, чтобы использовать +`calibrationTarget`/сохранённый `space`. `smoke_vacuum_firstuse.mjs` +(единственный смок, гоняющий `_vacApplyCalibrationProposal(true)`) не ловит +это, потому что его сценарий однопространственный: `dockSpace === route.space` +там всегда, поэтому баг структурно не может проявиться. `smoke_vacuum_multifloor.mjs` +не трогает калибровку вообще, только рендер уже откалиброванных routes. + +**Почему в скоупе и почему это не Medium.** AC8 — нормативный, явно +перечисленный критерий приёмки задачи, и путь, в котором он ломается +(«высокий residual → ручная донастройка»), не редкий частный случай: +это штатная ветка калибровки, которую спецификация специально ввела для +недостаточно точного auto-calibration (§9.4, риск-таблица §19 «Recalibration +портит trails»). Обходной путь пользователю недоступен без документации +(диалог визуально откроется, но на неправильном этаже, что почти наверняка +собьёт с толку админа, а не подскажет переключить пространство вручную). + +**Что нужно поправить.** Сохранить `space` в `_vacCalConfirm` (типовой узел +`{markerId, source, mapId, routeId?, space, matrix, rooms, error}` в обоих +местах — интерфейс `host._vacCalConfirm` объявлен в +`src/houseplan-card.ts:2511` и в `HouseplanEditorHostPort` +(`src/houseplan-editor-runtime.ts`) — и коммитить именно его в +`_vacApplyCalibrationProposal`, как это уже делает `_vacStartFit`. + +## Находка 2 (Medium, в скоупе) — §9.3 «Добавить источник карты» не реализован + +**Файл:** `src/editors/vacuum-maps-section.ts` (весь модуль). Кнопка +«Добавить текущую карту» (`addCurrent`, стр. 70-81) — единственный способ +завести route; отдельного выбора camera source (§9.3 ТЗ: «переиспользует +capability picker и ленивую секцию "Все камеры"») в блоке нет. + +Это прямо признано автором в комментарии реализации («Отступления от ТЗ», +п.1) и подтверждено чтением файла — других путей создания route, кроме через +текущий root `source`, не существует. Проблема не гипотетическая: тело +issue #162 перечисляет это вторым обязательным сценарием дословно — +«Поддерживаются оба варианта интеграций: один источник с меняющимся `map_id` +и отдельная camera entity на каждую карту» — и §5.2 ТЗ включает «явный выбор +другой camera source через существующий picker» в список обязательного +scope Device editor. + +Обходной путь существует (сменить root `source` на вторую камеру, добавить +её карту, затем при необходимости вернуть root обратно) и он функционально +работает, потому что route хранит свой `source` независимо от root. Поэтому +это не блокирует рендер/резолвер (AC4, AC5, AC7 проверяют resolution уже +настроенных multi-source routes и проходят), но заявленный сценарий +настройки «с нуля второй камерой, не трогая первую» не имеет прямого пути в +UI — только обходной, который нигде не объяснён пользователю. + +**Почему Medium, а не High:** нет потери данных, нет неверного отображения; +это пробел в scope UI с рабочим (хоть и не описанным) обходным путём, а не +неверное поведение уже настроенного routing. + +**Почему в скоупе задачи, а не отдельный issue:** §9.3 и часть §5.2 — это +нормативный текст ТЗ, утверждённого на spec-review r2 этого же issue, и один +из двух сценариев, названных в оригинальном теле #162. Это не соседнее +поведение — это заявленная, но недоделанная часть текущей задачи. + +## Что проверено и корректно + +- **Резолвер и его питоновское зеркало** (`vacuum-routes.ts` / + `vacuum_routes.py`) синхронны построчно: `resolveRoute`/`resolve_route`, + `adoptLegacyRun`/`adopt_legacy_run`, `validateMarkerRoutes`/ + `validate_marker_routes`, `effectiveRoutes`/`effective_routes` — идентичная + логика, идентичный порядок веток. Ambiguity — по `routeIds`, не по первому + совпадению (AC5, AC14, оба спот-чек мутанта поймали регрессию). +- **Рендер больше не фильтрует по пространству дока** — `_renderVacuums` + теперь получает `this._renderDevices` (всех устройств) плюс `spaceId` + текущего рисуемого пространства, и раздельно решает `live`/`previous` через + `planVacuumOverlay` по `route.space`, а не по `d.space`. Вызывается ровно + один раз на кадр (единственный call site, + `src/houseplan-card.ts:11794`), поэтому многократного пересчёта на + несколько пространств одновременно нет. Мутант + `vacuum-overlay-back-to-the-dock-space-filter`, возвращающий фильтр по + док-пространству, красит `smoke_vacuum_multifloor` — лично перепроверил. +- **Legacy-путь не переписывает конфиг сам по себе**: `effectiveRoutes`/ + `effective_routes` только читают `calibration`, ничего не пишут; явная + конверсия (`convertLegacyRoutes`) — all-or-nothing, требует exact source + (§7.3). Прочитано, логика соответствует. +- **`adoptLegacyRun`/`adopt_legacy_run`** — ровно три исхода (unique/ + orphan/ambiguous), отбор по `rootSource` отключается при его отсутствии, как + зафиксировано в правке spec-review r1 (§11.3.1); оба зеркала идентичны. +- **Backend recorder** (`trails.py`) подписывается на объединение всех route + source (не только root), `_route_id` находит совпадающий route по + `(source, map_id)`, `same_run_identity` разводит запуски по `route_id`, если + он есть, иначе — по `map_id` (legacy-совместимость сохранена буквально). + `drop_unknown_routes` вызывается из `async_purge_orphans`, которая, в свою + очередь, вызывается при config/set (websocket_api.py:1661) и после import — + то есть удаление space/route действительно чистит связанные runs в той же + логической операции (AC16), а не когда-нибудь потом. +- **`validate_marker_vacuum_routes`** — change-aware (не блокирует + непричастный Save при уже сломанных legacy/future routes), подключена к + `config/set` (оба места, `websocket_api.py:1582,1941`), `import_export.py` + (полный и частичный import). Прочитано и сверено с описанием §7.2 ТЗ. +- **`space-deletion.ts`** — считает `routeMarkerIds`/`routeCount` для чужих + маркеров, ссылающихся на удаляемое пространство, и вырезает только их + routes, не трогая dock/другие routes того же маркера (AC16, читал код, + подтверждено). +- **Ratchet ядровых файлов** соблюдён: `houseplan-card.ts` 13605/13659, + `houseplan-editor-runtime.ts` 14321/14323 (`wc -l`, сверено лично) — весь + новый код в `vacuum-routes.ts`, `vacuum-route-edit.ts`, + `vacuum-maps-section.ts`. +- **Trailers и changelog**: все 11 коммитов несут `Issue: #162` и корректный + `User-Visible`; каждый коммит с `User-Visible: yes` + (`c38501ef`, `ca6ebc30`, `567b4568`) правит оба changelog в себе самом — + сверено по `git show --stat`. +- **`node scripts/check-docs.mjs`** зелёный на текущем дереве — отпечаток + скриншотов не устарел. +- **RU/EN i18n parity** для новых ключей `vac.route_*`/`vac.routes_*` + (лениво, `i18n/support/{en,ru}.json`) и `vac.route_warn_*` (основной + словарь, обязателен для View) — точечно сверил наличие и параллельность. + +## Что не проверял и почему + +- **Полный HA harness** (`test_ha_*.py`, включая новые + `test_ha_import_export.py::issue_162_*`) — недоступен в этой песочнице + (`pytest_homeassistant_custom_component` не устанавливается, канон — + Linux CI/WSL, см. AGENTS.md). Автор указал то же ограничение в + комментарии реализации и привёл отдельный изолированный прогон логики + экспорта; сам этот отдельный прогон я не видел (не приложен как лог), но + логика `import_export.py` (проверка cross-space routes, remap на + импортируемый space) прочитана и не вызывает вопросов. +- **AC11 (reload/warm remount)** — принимаю аргумент автора (маршрутизация + пересчитывается из конфига на каждый кадр, состояние между рендерами не + хранится) как правдоподобный по чтению кода: `facts.set('vacuum:', …)` + пересобирается в `render()` безусловно, `resolveRoute` — чистая функция от + текущего конфига и снапшота состояний, ничего не кэширует между вызовами + card. Backend-часть continuity покрыта существующими + `test_resume_fails_closed_on_clock_rollback_and_survives_restart_shape` + + новыми `test_route_run_does_not_resume_into_another_route`/ + `test_same_route_keeps_one_run`. Отдельный E2E-смок на reload/remount не + считаю обязательным добавлять в рамках этого возврата — но если ревью после + находки 1 потребует более широкой правки калибровки, стоит заодно + зафиксировать это явным тестом, раз уж придётся трогать соседний код. +- **Golden и performance-профили** — не гонял; вне AC этого раунда, canonical + запуск — пре-бета (см. таблицу гейтов выше). +- **`docs/images/screenshots.json`** — не сверял побайтово с реальными PNG, + доверился `check-docs.mjs` (зелёный). +- **Остальные 20 файлов из «прямого совпадения» смок-селектора** (кнопка + danger-confirm, config writer, редакторские вкладки и т.п.) — не гонял; + пересечение с изменённым кодом только по общей инфраструктуре редактора + (`_config`, `_saveConfig`, `_model`, `_confirmDanger`), не по vacuum-логике. +- **37 «слабых связей»** смок-селектора — не гонял ни одного; ни одно имя + (`_model`, `_config`, `_saveConfig`) не указывает на код, которого коснулся + diff по существу. + +## Итог + +Возврат автору. Обе находки — в скоупе текущей задачи и чинятся в ней же +(процесс #202): отдельный issue не заводится. Находка 1 (High) требует +правки перед тем, как задача может стать зелёной; находка 2 (Medium) может +быть закрыта в том же коммите или отдельным, но в этой же ветке. + +--- + + + +## Материал раунда + +- Ветка: `issue/162-vacuum-map-space-routing`, коммит `4032b810b72b` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `f03a4fac82468b33357acb14bf76153213958a2a` + ``` + git log --all --format='%H %T' | grep f03a4fac8246 + ``` +- ТЗ `docs/specs/162-vacuum-map-space-routing.md`, блоб `a8edcfbe6c37700095f6f8582258eac4fcb58ad9` + ``` + git log --all --find-object=a8edcfbe6c37700095f6f8582258eac4fcb58ad9 -- docs/specs/162-vacuum-map-space-routing.md + ```