diff --git a/docs/reviews/CODE-REVIEW-520-r2.md b/docs/reviews/CODE-REVIEW-520-r2.md new file mode 100644 index 00000000..2475e3d1 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-520-r2.md @@ -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= + --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, теперь дважды измеренный +двумя независимыми прогонами — наследуется. + +--- + + + +## Материал раунда + +- Ветка: `issue/520-first-frame`, коммит `462b56453cd6948c083e62ad8c92ec29aeb022b2`. +- Дерево материала: `d7de184623e6a2759c67f56816b78f38166eb183`. +- Тело issue на момент ревью: sha256 `4524f04412...` (см. «Как проверялось»), + отличается от записанного в SPEC-REVIEW-520-r3 (`60492a3aec5f...`) — + находка по #517, разобрана в тексте документа, возврата в spec не + требует. +- Вердикт этого документа: жёлтый · High 0 · Medium 1 (в скоупе) + +--- + + + +## Материал раунда + +- Ветка: `issue/520-first-frame`, коммит `462b56453cd6` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `d7de184623e6a2759c67f56816b78f38166eb183` + ``` + git log --all --format='%H %T' | grep d7de184623e6 + ``` +- Тело issue: `e27aabb67c6fac2d56a562dfb03405fb232615db4790506469cc89ee5993501c` +- Вердикт конвейера: `yellow` · High 0