From 87f19941a3280dbaa01d8fb33978286aacfc1f20 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Tue, 8 Sep 2026 21:05:48 +0000 Subject: [PATCH] docs: review document for #490 Issue: #490 User-Visible: no --- docs/reviews/CODE-REVIEW-490-r1.md | 219 +++++++++++++++++++++++++++++ 1 file changed, 219 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-490-r1.md diff --git a/docs/reviews/CODE-REVIEW-490-r1.md b/docs/reviews/CODE-REVIEW-490-r1.md new file mode 100644 index 00000000..2cfb677d --- /dev/null +++ b/docs/reviews/CODE-REVIEW-490-r1.md @@ -0,0 +1,219 @@ +# CODE-REVIEW-490-r1 + +- **Issue:** #490 — Атомарный recovery и live-состояние сводной панели +- **Этап:** code (PROCESS.md §2.7) +- **Заход:** r1 · блокирующих циклов израсходовано 0 из 4 +- **SHA реализации:** `c6e6660d483c9297fc5fa86e9f9b5a7ad82a85a5` (ветка `issue/490-summary-recovery-live-state`) +- **ТЗ:** `docs/specs/490-summary-recovery-live-state.md`, ревью ТЗ зелёное на `95a34f73` (см. комментарий issue) +- **Диапазон:** `git diff origin/dev...HEAD` (57 файлов, 1179+/429-), из них продуктовый код: + `src/summary-panel.ts`, `src/summary-panel-runtime-loaded.ts`, `src/summary-panel-host.ts`, + `src/houseplan-card.ts` (2 строки), тесты `test/summary-panel.test.mjs`, + `test/render-device-snapshot.test.mjs`, новый `demo/smoke_summary_panel.mjs` (recovery/live + сценарии добавлены к существующему UI-смоку), `scripts/smoke-links.mjs`, changelog x2, dist/бандлы. + +## Скоуп + +Задача устраняет F1 (lost-ACK recovery устанавливал только `rev`, но старый `candidate`) и +F2 (entity-источники сводной панели не входили в render dependency set). Это единственная +задача в этом заходе; ТЗ уже прошло отдельное зелёное ревью, здесь оценивается реализация +против AC1–AC6. + +## Как проверялось + +Зелёного Validate на `c6e6660d` не нашёл (прогон не завершён/не найден), поэтому дешёвые гейты +прогнал сам: + +| Гейт | Результат | +|---|---| +| `npx tsc --noEmit` | green, без ошибок | +| `npm test` | green — 2276 pass, 0 fail, 1 skip (Node 22.23.2, Linux) | +| `npm run build` + сверка dist/custom_components/demo-srv | green — рабочее дерево после build/`node scripts/bundle-sync.mjs` осталось чистым (без diff к закоммиченным трём копиям бандла) | +| `node scripts/check-docs.mjs` | ERROR: «screenshot source fingerprint is stale ... (#479)» — это известный, помеченный самим инструментом долг #479 (отпечаток считается по всему `src/**`, любая фронтенд-правка делает его устаревшим); не регрессия этой задачи, отдельно не блокирую | +| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | прогнал вывод, см. ниже | +| `node demo/smoke_summary_panel.mjs` | green (`OK`) — включает новые recovery/live сценарии | +| `node demo/smoke_render_invalidation.mjs` | green — прямое совпадение по `entityIds` | +| `node demo/smoke_device_position_history.mjs` | green — прямое совпадение по `_adoptStructuralResponses` | +| `node demo/smoke_danger_confirmation.mjs` | green — прямое совпадение по `_adoptStructuralResponses` | +| `npm run invariants` | не запускал — diff не трогает рёбра/толщину/`layout`/`marker.space`/`open_spans` | +| `python -m pytest tests_backend` | не запускал — `custom_components/**/*.py` не тронут (только фронтенд-бандл) | +| golden/performance полные наборы | не запускал — это предрелизный гейт (PROCESS.md §8), diff не меняет геометрию/стили/слои рендера, только текстовые значения существующего блока | + +`smoke-select` дал 22 «прямых совпадения» и 58 «слабых связей» (общее имя `_maybeRebuildDevices`/`_model`/`_cfgRev` — решение ревьюера). Прогнал те прямые совпадения, что относятся к затронутым сиротным символам (`entityIds`, `_adoptStructuralResponses`); остальные прямые совпадения (`_cfgContentFingerprint`, `_restoreZoom`, `SpaceModel` и т.д.) относятся к путям, которые эта задача не меняла (сами функции идентичны на `origin/dev`, изменился только один новый вызывающий), и уже покрыты общим `npm test`/`tsc`. + +Читал код (не исполнял отдельно) `_adoptStructuralResponses`, `_beginContinuityCandidate`, +`VisualContinuityController` (`src/visual-continuity.ts`), `ContentSigner.prepareImage/display` +(`src/signing.ts`), три существующих вызывающих места adoption-seam (`_loadFromServer`, +`_reloadConfigOnly`, `space-card.ts:718`) — чтобы сравнить их с новым, четвёртым, в +`summary-panel-runtime-loaded.ts`. + +## Находки + +### M1 — Recovery-адаптация обходит asset-readiness gate и continuity-overlay (в скоупе, Medium) + +`src/summary-panel-runtime-loaded.ts:439-452` (метод `saveDialog()`, ветка lost-ACK): + +```ts +const authoritative = await this.host._getAuthoritativeConfig(); +const confirmed = confirmedSummaryPanelWriteRecovery(authoritative, draft); +if (!confirmed) throw writeError; +const visibleSpace = this.host._space; +this.host._adoptStructuralResponses(authoritative); +void this.host._syncDecorAssets(confirmed.config).catch(() => undefined); +this.host._adoptInitialSpace(this.host._model, true); +this.host._cacheSnapshot(); +if (this.host._space !== visibleSpace) this.host._restoreZoom(); +this.host._regSignature = ''; +this.host._maybeRebuildDevices(); +recovered = true; +``` + +Это четвёртый вызывающий `_adoptStructuralResponses` в кодовой базе. Три существующих +(`houseplan-card.ts:4286` `_loadFromServer`, `houseplan-card.ts:4467` `_reloadConfigOnly`, +`space-card.ts:718`) все делают одно и то же перед вызовом adoption-seam: + +```ts +if (configChanged && !await this._signer.prepareImage( + this.hass, this._candidateBackdrop(candidateConfig), +)) { + this._continuity.note('asset-failed'); + this._scheduleLoadRetry(true); + return; // старый _serverCfg остаётся на экране, пока фон не готов +} +``` + +`ContentSigner.prepareImage` — «Sign and decode a protected image before a structural +candidate adopts it. The current frame can therefore keep its old backdrop for the whole +wait» (`src/signing.ts:350-353`). Новый путь в `saveDialog()` этот шаг не делает и не +проверяет `configChanged` вовсе — `_adoptStructuralResponses` вызывается безусловно. +Он также не вызывает `_beginContinuityCandidate(...)` (recovery-overlay, #73/#451) и не +вызывает `_resumePendingNavMode()`, которые есть во всех трёх других местах. + +**Почему это задевает именно эту задачу.** ТЗ §6.2 прямо требует: «Если authoritative +adoption обнаруживает новую структурную базу, сохраняются действующие правила сброса stale +history/drag/cache и continuity #73/#451. Recovery панели не получает собственного обходного +варианта этих правил.» История/drag/cache сброс действительно сохранён — он находится внутри +самого `_adoptStructuralResponses` (geometryHistory, devicePositionHistory, drag, camera — +проверил чтением, это общий код, использованный без изменений). Но asset-readiness gate и +recovery-overlay — тоже часть тех же continuity-правил (#73/#451, тот же файл +`visual-continuity.ts`, та же авторская формулировка «keep its old backdrop for the whole +wait») — и именно по ним recovery панели завела свой обходной вариант: единственный из +четырёх вызывающих, который не ждёт `prepareImage`. + +**Сценарий отказа.** Клиент A открывает диалог сводной панели, сохраняет. Ответ на +`config/set` теряется по транспорту после того, как сервер его принял. Параллельно клиент B +успевает сохранить несвязанную структурную правку — например, заменить фоновое изображение +текущего пространства (`spaces[i].bg.href`) — именно такой параллельный несвязанный write +и есть предмет AC1/AC2. Recovery клиента A подтверждает lost-ACK (`confirmedSummaryPanelWriteRecovery` +проверяет только совпадение `summary_panel`, остальная часть документа — включая `bg.href` — +принимается «как есть») и вызывает `_adoptStructuralResponses(authoritative)` напрямую. +Новый `bg.href` ещё не подписан (`ContentSigner.signed` не содержит записи для нового пути) — +`_display()` для непобранного URL возвращает `''` («a signature we hold and still trust, else +nothing», `signing.ts:161-179`), поэтому фон плана на один кадр/до ближайшего подписания +пропадает (пустой src), вместо того чтобы держать старый фон, как это гарантируют три других +пути. Само подписание триггерится тем же вызовом `_display()` и самовосстанавливается на +следующем цикле — это не постоянная поломка, а видимый, но переходный дефект рендера именно +в момент, который ТЗ описывает как основной (recovery при параллельной структурной правке). + +**Проверено чтением, не исполнением** — воспроизвести смоком дорого (нужен реальный +`ContentSigner`/подписанный URL в browser harness), а `demo/smoke_summary_panel.mjs` меняет +только `spaces[0].title` и `settings.concurrent_guard_490` — ни то, ни другое не задевает +`ContentSigner`, поэтому текущий смок этот путь не покрывает и не может его поймать. + +**Серьёзность:** Medium, в скоупе задачи (сам предмет AC1/AC2 — атомарное принятие «всего» +документа при recovery, включая структурные поля вроде фона). Не блокирует (не High): не +ломает данные и не искажает `summary_panel`, эффект переходный и самовосстанавливающийся, +требует узкого стечения (lost ACK + именно в этом окне конкурентная правка фона). Чинится +добавлением `configChanged`-гейта на `prepareImage` (и, по аналогии с тремя другими местами, +`_beginContinuityCandidate`) перед вызовом `_adoptStructuralResponses` в этой ветке — +без выдумывания нового поведения, тем же способом, что и в трёх существующих вызывающих. + +## Что проверено и корректно + +- **AC1/AC2 (атомарность и следующая запись).** `confirmedSummaryPanelWriteRecovery` + (`src/summary-panel.ts`) требует одновременно: полный `ServerConfig` (`Array.isArray(config.spaces)`), + числовой неотрицательный safe-integer `rev`, точное совпадение сохранённой `summary_panel` + с draft (`sameSummaryPanel`, JSON-эквивалентность). При выполнении — весь `authoritative` + документ и `rev` уходят через общий `_adoptStructuralResponses`, а не собираются вручную — + это закрывает риск «принят только rev или только config» из ТЗ §11. Подтверждено и unit-тестом + (`test/summary-panel.test.mjs`, матрица из 7 отрицательных и 1 положительного случая) и + browser-смоком `demo/smoke_summary_panel.mjs` (`recoveryAdoptsWholeConfig`, + `recoveryAdoptsRevision`, `nextWriteUsesRecoveredRevision`, + `nextWritePreservesConcurrentChange`) — прогнал сам, green. Тест умеет падать: ассерты + сверяют конкретные строки (`'Concurrent title kept'`, `'kept'`) и конкретную revision-арифметику + (`writes[0].expected_rev + 2`), а не факт отсутствия исключения. +- **AC3 (настоящий конфликт).** Ветка `catch { throw writeError; }` вокруг recovery-попытки — + любой сбой самого чтения (`_getAuthoritativeConfig` бросает) или отказ `confirmedSummaryPanelWriteRecovery` + (панель отличается / config или rev невалидны) откатывается к исходной ошибке записи, диалог + не закрывается. Смок `trueConflictStaysOpen` (принудительный `rejectConflict = true` на каждой + попытке) — green, и явно проверяет, что предыдущая («Second summary write») сохранённая версия + осталась в `_serverCfg`, а не смешалась со stale candidate. +- **AC4/AC5 (live-источники и fast path).** `summaryPanelEntityIds()` — чистая функция, не + читает `hass`, не мутирует конфиг, схлопывает дубликаты, режется по `runnableSummaryPanel` + (только поддерживаемая v1), игнорирует локальную видимость/scope, что соответствует ТЗ §7.1 + дословно («число ограничено схемой» — до 200 значений по `SUMMARY_PANEL_SCHEMA`, что и + проверяет unit-тест на hidden/other-scope/system/unsupported случаях). В + `houseplan-card.ts:4623` результат добавлен в тот же `entityIds` render-snapshot набор, что и + устройства/комнаты/декор — второго listener/timer не создано (проверено чтением всего файла + на предмет других мест, где могла бы завестись отдельная подписка). Live-переходы value → + unavailable → recovered → missing → present и счётчики рендера/geometry-инвариантность + проверены смоком (`liveState` секция) — green, включая `unrelatedTickSkipped: true`, + `relevantTickRenderedOnce: true`, `stateTicksKeepGeometry: true`. +- **Кеш-инвалидация снапшота.** `_capturedSnapshotSequence = -1` добавлен именно в момент + ленивой загрузки `LoadedSummaryPanelRuntime` (`connectedCallback`, `houseplan-card.ts:2613`) — + без него уже захваченный снапшот (`_hassSequence` не поменялся) не подхватил бы вновь + появившиеся `entityIds()` до следующего изменения hass. В самом recovery-пути отдельный сброс + не нужен: `_adoptStructuralResponses` переприсваивает `_serverCfg` новой ссылкой при + structural-change, Lit-реактивность ловит это в `willUpdate` (`houseplan-card.ts:4084-4088`) и + инкрементирует `_cfgEpoch`, что и инвалидирует кеш снапшота обычным путём — проверено чтением. +- **AC6 (совместимость).** Диф не трогает EN/RU/DE/FR словари, не добавляет новых сетевых + запросов в steady state (единственный новый `config/get` — тот же самый вызов, что и раньше, + просто с другим следствием), не меняет схему/API. `test/render-device-snapshot.test.mjs` + подтверждает, что `_summary?.entityIds()` вошёл именно в существующую сборку снапшота, а не + завёл параллельную. +- **Трейлеры и changelog.** `Issue: #490` на всех трёх коммитах, `User-Visible: yes` на + коммите реализации `c6e6660d`, и `docs/CHANGELOG.md`+`docs/CHANGELOG.ru.md` правлены в этом + же коммите (не отдельным) — соответствует требованию. +- **Единственный источник числа.** Значение summary-only сущности показывается только в одной + строке сводной панели; диф не вводит второе место, где то же значение показывалось бы снова + (никакой другой слой плана не подписан на источники, которых нет в общем `entityIds`) — не + вижу дублирующего числа, которое требовало бы отдельного разбора по «одно число — один + источник». + +## Что не проверял + +- Полный `golden:verify` — diff не меняет геометрию/раскладку/стили, только текстовые + значения существующего блока и внутреннюю логику recovery; посчитал непропорциональным для + этой правки. +- Windows-специфичные 3 падения `npm test`, о которых пишет автор (#479) — не переисполнял на + Windows, полагаюсь на то, что Linux-прогон (канонический CI job) у меня самого дал 0 fail. +- `npm run invariants` — diff не трогает геометрию/рёбра/`layout`/`marker.space`/`open_spans`. +- `python -m pytest tests_backend` — `custom_components/**/*.py` не в diff (только + сгенерированный фронтенд-бандл под `custom_components/houseplan/frontend/`). +- Полную матрицу `demo/smoke_*.mjs` (232 файла) — прогнал только прямые совпадения, относящиеся + к реально изменённым символам (`entityIds`, `_adoptStructuralResponses`); остальные 20 «прямых» + и все «слабые» совпадения относятся к функциям, чьи тела не менялись в этом diff (только один + новый вызывающий), и уже покрыты `tsc`+`npm test`+`build`. +- Живое ручное тестирование в браузере HA — недоступно в этом цикле; вывод построен на + автотестах и чтении кода. + +## Вердикт + +Жёлтый: единственная находка (M1) — Medium в скоупе задачи, чинится в этой же ветке без +отдельного issue (решение владельца 2026-08-19, #202). AC1–AC6 в остальном подтверждены +тестами, которые умеют падать, и чтением кода там, где смок не достаёт (сам M1). + +--- + + + +## Материал раунда + +- Ветка: `issue/490-summary-recovery-live-state`, коммит `c6e6660d483c` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `fc7364e6d21b43b3f08c757a8ce0d5d5ac9b48c2` + ``` + git log --all --format='%H %T' | grep fc7364e6d21b + ``` +- ТЗ `docs/specs/490-summary-recovery-live-state.md`, блоб `d4bfd4ea63f1b2ce1cb255b2764fe14d25661df7` + ``` + git log --all --find-object=d4bfd4ea63f1b2ce1cb255b2764fe14d25661df7 -- docs/specs/490-summary-recovery-live-state.md + ```