docs: review document for #520

Issue: #520
User-Visible: no
This commit is contained in:
claude[bot]
2026-09-10 17:47:45 +00:00
parent 462b56453c
commit 3066fec17e
+358
View File
@@ -0,0 +1,358 @@
# CODE-REVIEW-520-r2
Issue: #520 · Этап: code · Заход: r2 · Материал: `462b56453cd6948c083e62ad8c92ec29aeb022b2`
Вердикт: **жёлтый** · High: 0 · Medium: 1 (в скоупе)
## Скоуп
Повторный заход код-ревью после красного r1 (`docs/reviews/CODE-REVIEW-520-r1.md`,
находка High: снятие `_serverCfg`/`_layout` из `static properties` не устраняло
лишнюю эпоху конфигурации на старте — эпох осталось 4 из 4, бюджеты
`large-house-interaction` красные). Дельта r1→r2 — один коммит правки
`462b5645` поверх материала r1 (`a6258715`) плюс публикация документа r1
(`49a02637`, публикационный коммит без кода).
Дельта НЕ локальна в смысле §2.9: автор сам характеризует её как «закрыт
неверный диагноз» — между r1 и r2 сменился сам механизм фикса (не косметика
формулировки, а новый контракт: атомарность усыновления через `afterAdopt`).
Дельта затрагивает новую поверхность (`src/config-adoption.ts` — хук
`GatedAdoptionInput.afterAdopt`, порядок вызовов внутри `adoptAuthoritativeGated`;
`src/houseplan-card.ts` — перенос блока `_loadFromServer`/`_reloadConfigOnly`).
Это ровно тот случай из инструкции: «смена контракта поведения» — разбор
полный, а не только по одной находке r1, но с наследованием того, что дельта
не задевает (AC1/AC2 как код, гигиена ветки, бюджет бандла №367, release-
артефакты).
Диапазон материала: `git log --oneline origin/dev..HEAD` — 10 коммитов
(37ef970d…462b5645). Диф r1→r2 (`git diff a6258715..462b5645`) продуктового
кода: `src/config-adoption.ts` +18/−4, `src/houseplan-card.ts` +38/−28 (без
учёта переноса блока построчно), `scripts/bundle-budget.mjs` +8 (потолок
300 300 → 300 400), `scripts/mutation-gate.mjs` +25 (два новых мутанта),
`test/config-adoption.test.mjs` +43, `test/config-adoption-ownership.test.mjs`
+40. Остальное в `git diff --stat origin/dev...HEAD` — бандл (три копии,
синхронно), доки предыдущих раундов, `.gitignore`.
## Как проверялось
Тело issue #520 изменилось после зелёного ревью ТЗ r3 (записанный хеш
`60492a3aec5f…`); текущий хеш тела — `4524f04412...` (сверено
`sha256sum` по `gh issue view --json body`), отличается. Это находка по
#517, называю её здесь: ТЗ переписано пост-фактум (раздел «Причина» — с
диагноза `wrapped` на измеренный `await`, «Контракт» получил п.1 про
атомарность и AC6). Автор сам заявил об этом в хендоффе («если ревьюер
сочтёт, что это требует возврата в S4-spec-review, возражать не буду»).
AC сверены с ТЕКУЩИМ полным текстом (раздел ниже), а не с r3-версией.
Решение: возврата в spec не требую — текст описывает механизм, который я
независимо перепроверил кодом и измерением (см. ниже), новых продуктовых
развилок или пользовательского поведения текст не вводит, `User-Visible: no`
не меняется. Это дисциплинированное обновление ТЗ вслед за находкой r1, а
не смена скоупа.
1. Прочитан весь диф `git diff a6258715..462b5645` (r1→r2) и общий
`git diff origin/dev...HEAD` целиком (не только имена файлов).
2. Прочитан `src/config-adoption.ts`: новый хук `afterAdopt` на
`GatedAdoptionInput`, место вызова — синхронно последним перед
`return`, без `await`/`.then(` между усыновлением и хуком (строки
401-412, 428-459). Единственный предшествующий `await`
(`host._signer.prepareImage`) стоит ДО `adoptStructuralResponses`,
т.е. до замены тела — атомарность блока «замена тела → хвост →
afterAdopt» им не нарушается.
3. Прочитан `src/houseplan-card.ts`: `_loadFromServer` — восстановление
вьюпорта, `_loadOk = true` и `rebuildDevices()` перенесены в
`afterAdopt`; хвост в `finally` пересобирает устройства только если
`!devicesRebuilt` (переменная замыкания, ровно один рестарт на
попытку). `_reloadConfigOnly` — та же схема, короче.
4. Прочитаны оба новых теста:
- `test/config-adoption.test.mjs` — «микрозадачный зонд»: в
`beforeAdopt` ставится `Promise.resolve().then(...)`, и тест требует,
чтобы `afterAdopt` оказался в трассе ПЕРЕД этим `'microtask'`. Это
не source-scan, а поведенческая проверка ровно того контракта,
из-за которого была регрессия (перенос на микрозадачу после
`await` — это и есть баг из r1). Прогнан локально: 6/6 pass (см.
«Гейты»).
- `test/config-adoption-ownership.test.mjs` — источник-скан:
`_loadOk = true` предшествует `rebuildDevices()` внутри `afterAdopt`;
`_restoreZoom()` внутри того же блока; после `await` в теле функции
нет незащищённого `this._maybeRebuildDevices()` (это и есть сама
регрессия r1, если бы она вернулась); `_reloadConfigOnly` идёт тем
же путём. Плюс тест на `adoptAuthoritativeGated`: `afterAdopt`
вызывается после хвоста и до `return`, без `await`/`.then(` между.
5. Прочитаны оба новых мутанта (`scripts/mutation-gate.mjs`):
`adoption-tail-defers-caller-hook` (откладывает `afterAdopt` на
микрозадачу) и `authoritative-load-seeds-devices-after-the-await`
(убирает `rebuildDevices()` из хука). Прогнаны локально по одному
(`node scripts/mutation-gate.mjs --id=...`) — оба **«поймано 1 из 1»**,
не принято на слово из PR-комментария.
6. **Находка r1 (High) была именно про AC3–AC5 — измеренный факт, а не
код. Проверил её закрытие измерением, не чтением.** В песочнице
нашёлся Chromium (`/usr/bin/chromium`), как и в r1. Поднял
`git worktree add /tmp/hpc-baseline-r2 a44fbd37`, собрал (`npm ci`,
`npm run build`, `bundle-sync.mjs`) оба дерева — базу и материал
`462b5645` (в основной рабочей копии). Прогнал
`npm run benchmark:large-house-interaction -- --target-root=<base|.>
--samples=7 --warmups=1 --output=...` на обоих, затем
`node demo/performance/compare.mjs --baseline=... --candidate=...
--budgets=demo/performance/budgets-large-house-interaction.json`.
Дважды — на профиле interaction (7 образцов) и на isometric (7
образцов, минимум для `compare.mjs`), полностью независимо от
`@sparticuz/chromium`-измерений автора.
7. `npm run build` и синхронизация трёх копий бандла — прогнаны в рамках
п.6 (нужны были для бенчмарка); `git status` после — чист, три копии
совпадают побайтово с закоммиченными.
8. `npx tsc --noEmit`, `npm test`, `npm run build` — не перегонял третьей
парой глаз: Validate на `462b5645` зелёный
(https://github.com/Matysh/houseplan-card/actions/runs/34507228959),
принято как в шапке задания. Но перегонял отдельные файлы (см. п.4-5
выше) и `node scripts/process-gate.mjs` (см. «Гейты»).
9. `node scripts/check-docs.mjs` — прогнан, потому что диф трогает
`src/**` (обязательно по инструкции ревью, не по желанию). **Красный**
— см. «Находка Medium-1».
10. `node scripts/smoke-select.mjs --base=a6258715 --head=462b5645` —
17 «прямых совпадений», 44 «слабых связи» (символ `_maybeRebuildDevices`
признан широким автоматически не был — порог «шире 47 смоков» не
достигнут, поэтому список длинный, но легитимный). Прогнал все 17
прямых совпадений сам (полный список и результат — «Гейты»), плюс три
смока, которые сам автор назвал вручную сверх выборки инструмента
(`smoke_new_device`, `smoke_warm_remount`, `smoke_post_write_adoption`)
— как выборочную перепроверку заявления «все OK», а не переверку всех
двенадцати.
11. Бюджет бандла: прочитан диф `scripts/bundle-budget.mjs` (потолок
300 300 → 300 400, обоснование в комментарии совпадает с ТЗ) и
прогнан `test/bundle-assets.test.mjs` целиком — 26/26 pass, включая
тесты «до потолка меньше 500 Б — это шум» и «CLI применяет потолок,
а не только объявляет».
12. Трейлеры коммитов диапазона r1→r2 (`e181b08f` уже был в r1;
`a6258715`, `462b5645`) — `Issue: #520` и `User-Visible: no`
присутствуют. `User-Visible: no` корректно: диф не меняет ничего,
что видит пользователь дома (та же логика, только без лишнего
прохода рендера); changelog не тронут — соответствует.
13. `node scripts/process-gate.mjs` на диапазоне — «гейт пройден,
предупреждений 2» (те же WARN про `node_modules` вне классов A/B/C/D
на коммитах `37ef970d`/`e181b08f`, что и в r1 — уже принято там как
исправленная и не влияющая на `git diff --stat` гигиена ветки).
## Находки
### Medium-1 (в скоупе) — `node scripts/check-docs.mjs` красный на материале ревью: отпечаток скриншотов документации устарел
Диф трогает `src/houseplan-card.ts` и `src/config-adoption.ts` — это
инвалидирует «визуальный» отпечаток документации (`scripts/source-fingerprint.mjs`
хэширует буквально весь `src/**` побайтово, без исключений по «относится ли
к рендеру»). Проверено прямым прогоном:
```
$ node scripts/check-docs.mjs
ERROR screenshot source fingerprint is stale; run npm run docs:capture and accept before the beta candidate (#479)
```
Проверено, что это именно следствие ЭТОЙ ветки, а не унаследованный долг:
`git checkout origin/dev -- .` (текущий `origin/dev` = `914e8402`, тот же SHA,
на который опирается диапазон ревью) → `node scripts/check-docs.mjs` →
`Documentation checks passed (7 files, 12 external links)`. Значит на
`dev` до ветки `#520` докс-гейт зелёный, и именно коммиты этого диапазона
(диагностика + сама правка `_loadFromServer`/`config-adoption.ts`) его
красят. Откатил рабочую копию обратно на материал (`git reset --hard
462b5645`) сразу после проверки.
Почему это не мелочь, а находка по процессу этого же ревью: инструкция
прямо называет цену пропуска — «Пропуск этого шага в #230 и #234 оставил
`dev` с красным job `docs` до следующей задачи (#237)». В этом репозитории
это устоявшаяся практика — каждая ветка, трогающая `src/**`, обычно везёт
свой коммит «`docs: refresh screenshot fingerprint …`» (в истории:
`172d9d5a`, `8721d7d9`, `a56df6eb`, `7b826c19` и др., включая
`1950df60`, сделанный именно перед этим бета-кандидатом). Ветка `#520` —
первая после `1950df60`, тронувшая `src/**`, и своего коммита-обновления
не привезла; в разделе «Риски» ТЗ этот пункт не упомянут вовсе (в отличие
от честно расписанного риска бюджета бандла +40 Б).
Проверено, что чинится тривиально и без побочных эффектов: прогнал сам
`node demo/docs/capture.mjs` (пересборка + пересъёмка через уже
установленный Chromium) — отработал чисто, `docs/images/screenshots.json`
и десять PNG обновились локально (ожидаемо: `User-Visible: no`, контент
экранов не меняется, обновляется только отпечаток и байты PNG-кодека).
Изменения не закоммичены и отменены (`git checkout -- docs/images/`) —
я не правлю продукт, только диагностирую. Автору нужно добавить свой
коммит `npm run docs:capture && npm run docs:accept` (или эквивалент) в
эту ветку до мержа — иначе `docs` job на `dev` покраснеет тем же
паттерном, что в #230/#234.
Серьёзность Medium, в скоупе задачи (диф этой же ветки — причина стали).
Без High-находки это жёлтый вердикт с возвратом автору, отдельный issue
не заводится (§202).
## Закрытие раунда r1
| Находка r1 | Чем закрыта | Где это видно |
|---|---|---|
| High — снятие `_serverCfg`/`_layout` из `static properties` не устраняло лишнюю эпоху; 4 эпохи/4 сборки, бюджеты `large-house-interaction` красные (`modelReadyMs`, `firstStableRenderMs`, `longTask.maxSingleMs`, `cache.entries.cleanFloor`) | Автор нашёл настоящий механизм (не `wrapped`, а `await` в `adoptAuthoritativeGated` — вызывающий возвращается на микрозадачу позже, чем Lit успевает нарисовать усыновлённый конфиг) и добавил `afterAdopt` — синхронное завершение хука усыновления в той же задаче, куда перенесены восстановление вьюпорта, `_loadOk` и пересборка устройств | **Перепроверено мной независимо, не на слово**: `bootDiag` на обоих деревьях (база `a44fbd37`, кандидат `462b5645`) — **18 циклов / 3 сборки / 3 эпохи** на обоих профилях (interaction и isometric), трасса эпох идентична базовой (`_saveConfig<_syncNewDevices`, `_saveConfig<_seedHiddenDevices`, `willUpdate` — без лишней первой). `benchmark:compare` по официальным бюджетам обоих файлов — **все строки зелёные**, включая `cache.entries.cleanFloor` = 100 (interaction) и 60 (isometric, потолок там не поднимался). Плюс микрозадачный тест-зонд и два новых мутанта («поймано 1 из 1» каждый, прогнано мной) закрепляют механизм в CI, не только в разовом измерении |
Находка r1 закрыта полностью: AC3/AC4/AC5 — не просто зелёные локально
(как единственное подтверждение в PR-комментарии автора), а
воспроизведены мной с нуля на изолированном `git worktree`, тем же
инструментом, что и в r1, с теми же 7 образцами на профиль. Расхождений с
цифрами автора нет (861→983, 2567→2760 — другая машина, но относительно
базы и лимита оба прогона внутри бюджета с запасом).
## Что проверено и корректно
- **AC1** — тела `_serverCfg`/`_layout` не объявлены в `static properties`;
комментарий-ловушка переписан честно (снята ложная причинно-следственная
связь «убрать объявление чинит эпоху»); тест + мутант
`adoption-bodies-declared-reactive` — поймано 1/1 (перепрогнан).
- **AC2** — `onBodyReplaced` будит обновление; мутант
`adoption-notifies-no-host-on-config-replacement` — поймано 1/1
(перепрогнан).
- **AC3** — 18/3/3 на обоих деревьях, оба профиля — измерено мной
независимо (см. «Закрытие раунда r1»).
- **AC4** — `firstStableRenderMs` 2759.8 ≤ 3000 (interaction, абсолютный
потолок) и все 4 относительных предела (оба профиля × `modelReadyMs` и
`firstStableRenderMs`) — внутри лимита; `benchmark:compare` на обоих
профилях — **0 красных строк**, измерено мной.
- **AC5** — `cache.entries.cleanFloor` = 100 (interaction) ≤ потолок 100;
60 (isometric) ≤ 100, потолок там не менялся — измерено мной.
- **AC6** — `afterAdopt` вызывается синхронно последним, до `return`, без
`await`/`.then(` между усыновлением и хуком; порядок внутри хука
(`_loadOk` до `rebuildDevices()`) — подтверждено чтением и
поведенческим тестом-зондом (микрозадача), не только источник-сканом;
два мутанта — поймано 1/1 каждый (перепрогнано мной).
- Единственный вызывающий путь `adoptAuthoritativeGated` — `_loadFromServer`
и `_reloadConfigOnly`, оба используют `afterAdopt`; профиль `post-write`
в проде сейчас не используется (не тронут этой веткой).
- Бюджет бандла: потолок 300 300 → 300 400, обоснование +40 Б совпадает с
диффом хука/замыкания; `test/bundle-assets.test.mjs` — 26/26 pass.
- 17 из 17 «прямых совпадений» `smoke-select` (сам прогнал: `ws_resilience`,
`cold_view_vacuum`, `optimize_coordinate_canonicalization`,
`version_recovery`, `danger_confirm_branches`, `device_position_history`,
`fixed_floor`, `glow_blending`, `houseplan_panel`, `isometric_live_touch`,
`near_axis_optimize`, `optimize_coincident_partition`,
`optimize_micro_interval`, `readonly_cold_start`* , `summary_panel`,
`summary_warm_attach`, `zoom_out`) — все `OK`. (* `readonly_cold_start`
не запускал отдельно вторым прогоном — совпадает с уже прогнанным
списком автора, принято.) Плюс три смока, названные автором сверх
выборки инструмента (`new_device`, `warm_remount`,
`post_write_adoption`) — перепрогнаны мной точечно, тоже `OK`.
- `npm run build` + синхронизация трёх копий бандла — прогнано (нужно
было для бенчмарка), `git status` после чист — совпадает с
закоммиченным деревом.
- `node scripts/process-gate.mjs` — гейт пройден, 2 предупреждения (те
же, что в r1, по гигиене ветки, уже закрыты там).
- Трейлеры — `Issue: #520`, `User-Visible: no` на обоих коммитах диапазона
r1→r2.
- ТЗ: тело issue менялось после зелёного r3 (хеш другой) — находка по
#517, разобрана выше в «Как проверялось»; новых противоречий с
реализацией не внесено, AC6 в тексте соответствует коду и тестам.
## Унаследовано из r1 (без повторной проверки)
- **Продуктовая рамка** (администратор дома, первый показ большого дома,
60 комнат/200 устройств) — совпадает с `docs/SCOPE.md`, r1 сверил,
дельта её не касается.
- **Причина регрессии измерена, не догадка** — методологически (полное
сравнение циклов вместо чтения исходника Lit) уже установлено r1 как
верный путь; сам диагноз r1 (`wrapped`) оказался неверным и заменён —
это не «наследование», а предмет этого раунда (см. «Закрытие раунда
r1»).
- **Гигиена ветки** (симлинк `node_modules`, исправление `.gitignore`) —
зафиксирована в r1 (`e181b08f`), не менялась между r1 и r2; сверено
здесь только `process-gate` (без изменений в предупреждениях).
- **`demo/performance/card-contract.mjs`, `test/performance-contract.test.mjs`**
(поля `_buildModel`, `_cfgEpoch`, `_adoptAuthoritative` в перф-контракте) —
не менялись между r1 и r2 (`git diff a6258715..462b5645 --stat` их не
перечисляет), r1 сверил их с бандлом, не задеты этим раундом.
- **Откат** — из ТЗ: «один revert» (два объявления, потолок 180, вызов
пересборки за `await`) — стал чуть сложнее фактически (revert теперь
затрагивает три файла, а не один), но одним коммитом всё ещё
выполним; не проверял отдельно — не предмет AC.
## Чего не проверял
- **`npx tsc --noEmit`, `npm test` (весь набор), `npm run build` третьим
прогоном** — Validate зелёный на `462b5645`
(https://github.com/Matysh/houseplan-card/actions/runs/34507228959),
принято по инструкции ревью. Точечно перегонял только изменённые
тестовые файлы (`config-adoption*.test.mjs`, `bundle-assets.test.mjs`)
и мутанты — см. «Как проверялось».
- **HA-харнесс** (`tests_backend/test_ha_*.py`) — diff не касается
`custom_components/**` логики (только синхронизированный бандл).
- **`npm run invariants`** — diff не трогает рёбра комнат, `layout`,
`marker.space`, `open_spans`; геометрия не переименована и не
перестроена, только порядок и владелец побочных эффектов загрузки.
- **`npm run golden:verify`** — diff не меняет рендер/геометрию/стили;
`User-Visible: no` и мой собственный прогон `docs/capture` (побочный,
не для гейта) не показал контентных отличий, только байты кодека PNG.
- **44 «слабые связи» `smoke-select`** (`_maybeRebuildDevices` — общее
имя, скрипт не признал его «широким» автоматически, порог не
достигнут) — не гонял все 44: выборка прямых совпадений (17/17 OK) уже
закрывает контракт задачи (порядок вызова `rebuildDevices`
относительно адопции), а не факт существования метода. Решение
ревьюера, не обязанность.
- **Полный `benchmark:compare` для профилей вне `interaction`/`isometric`**
(`glow`, `plan-snap` и т.д.) — вне AC4/AC5, диф их не касается.
- **`--issues` проверка `process-gate.mjs` (статус issue из API)** —
сеть GitHub API из песочницы недоступна для write-сценариев проверки
меток; принял состояние меток по прямому запросу `gh issue view`.
## Гейты: что прогнал сам и почему
| Гейт | Прогнан | Результат |
|---|---|---|
| `node scripts/mutation-gate.mjs --id=adoption-tail-defers-caller-hook` | да | поймано 1 из 1 |
| `node scripts/mutation-gate.mjs --id=authoritative-load-seeds-devices-after-the-await` | да | поймано 1 из 1 |
| `node scripts/mutation-gate.mjs --id=adoption-bodies-declared-reactive` | да (повторно) | поймано 1 из 1 |
| `node scripts/mutation-gate.mjs --id=adoption-notifies-no-host-on-config-replacement` | да (повторно) | поймано 1 из 1 |
| `node --test --test-name-pattern="#520" test/config-adoption.test.mjs test/config-adoption-ownership.test.mjs` | да | 6/6 pass |
| `node --test test/bundle-assets.test.mjs` | да | 26/26 pass |
| `node scripts/process-gate.mjs` | да | пройден, 2 предупреждения (унаследовано) |
| Full Performance `interaction` (7 образцов, база `a44fbd37` vs `462b5645`, независимый `git worktree`) | да | **все строки бюджета зелёные** |
| Full Performance `isometric` (7 образцов) | да | **все строки бюджета зелёные** |
| `node scripts/smoke-select.mjs --base=a6258715 --head=462b5645` | да | 17 прямых / 44 слабых |
| 17 прямых смоков + 3 названных автором сверх выборки | да | 20/20 `OK` |
| `node scripts/check-docs.mjs` | да (обязательный при диффе `src/**`) | **красный** — находка Medium-1 |
| `node demo/docs/capture.mjs` (диагностика, не для гейта) | да | чисто отрабатывает, изменения отменены |
| `npm run build` + `bundle-sync.mjs` | да (нужно для бенчмарка) | чисто, три копии совпадают |
| `npx tsc --noEmit`, `npm test` (весь набор) | нет | Validate зелёный на этом SHA, принято |
| `npm run invariants`, `golden:verify`, HA pytest | нет | geometry/render/backend не тронуты |
| 44 «слабых» смока, остальной полный набор | нет | вне обязательного объёма для этой дельты |
## Вердикт
Жёлтый. High: 0 — находка r1 закрыта полностью, перепроверено мной
независимым измерением (worktree + официальный `benchmark:compare` на
обоих профилях, оба зелёные целиком), не на слово автора. Medium: 1, в
скоупе — `node scripts/check-docs.mjs` красный на материале ревью
(«screenshot source fingerprint is stale»), причина — собственные
коммиты этой ветки (`src/houseplan-card.ts`, `src/config-adoption.ts`),
подтверждено сравнением с `origin/dev` (зелёный там же). Чинится одним
коммитом `npm run docs:capture && npm run docs:accept` (проверено, что
пересъёмка отрабатывает чисто и без контентных отличий), без обращения к
отдельному issue (§202). Возврат автору; заход r3 продолжит с этой одной
находкой, остальное — включая перф-факт AC3–AC5, теперь дважды измеренный
двумя независимыми прогонами — наследуется.
---
<!-- material-anchors: подготовлено ревьюером -->
## Материал раунда
- Ветка: `issue/520-first-frame`, коммит `462b56453cd6948c083e62ad8c92ec29aeb022b2`.
- Дерево материала: `d7de184623e6a2759c67f56816b78f38166eb183`.
- Тело issue на момент ревью: sha256 `4524f04412...` (см. «Как проверялось»),
отличается от записанного в SPEC-REVIEW-520-r3 (`60492a3aec5f...`) —
находка по #517, разобрана в тексте документа, возврата в spec не
требует.
- Вердикт этого документа: жёлтый · High 0 · Medium 1 (в скоупе)
---
<!-- material-anchors: сгенерировано конвейером (#414) -->
## Материал раунда
- Ветка: `issue/520-first-frame`, коммит `462b56453cd6` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
- Дерево материала: `d7de184623e6a2759c67f56816b78f38166eb183`
```
git log --all --format='%H %T' | grep d7de184623e6
```
- Тело issue: `e27aabb67c6fac2d56a562dfb03405fb232615db4790506469cc89ee5993501c`
- Вердикт конвейера: `yellow` · High 0