diff --git a/docs/reviews/CODE-REVIEW-536-r1.md b/docs/reviews/CODE-REVIEW-536-r1.md new file mode 100644 index 00000000..d705cfcd --- /dev/null +++ b/docs/reviews/CODE-REVIEW-536-r1.md @@ -0,0 +1,140 @@ +# CODE-REVIEW-536-r1 + +**Issue:** #536 «Плашка версий не гарантирует своё исчезновение: контроллер меняет состояние, не прося перерисовку» +**Заход:** r1 · блокирующих циклов израсходовано 0 из 2 (лёгкий трек) +**Материал:** ветка `issue/536-banner-notify`, HEAD `1e1880f010495fe23197feddc64e33138445fc5d`, +база `origin/dev` = `05a2c68d`. Ровно один коммит в диапазоне `origin/dev..HEAD`. + +Предыдущая попытка запуска ревью на этом issue (комментарий «Ревью не запускалось») не +вынесла вердикта: конвейер сам перебазировал ветку на `dev` между хендоффом и стартом +Validate, гонка диспатча против ребейза, а не красный код. Кода никто не читал. Это +честно первый заход код-ревью, разбор — полный. + +## Скоуп + +Одна строка поведенческого кода в `src/version-recovery.ts` (`disconnect()`), плюс +свидетель (`test/version-recovery.test.mjs`, 2 новых теста), плюс регистрация мутанта +(`scripts/mutation-gate.mjs`), плюс пересборка трёх копий бандла и обновление отпечатка +скриншотов документации. Ровно то, что названо в ТЗ («Затронутые файлы»). + +## Как проверялось + +Прочитан `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md`, тело issue #536 целиком (постановка + +ТЗ), все комментарии (аналитика/ТЗ на одном ходу → зелёное ревью ТЗ r1 → хендофф → +гоночный false start → возврат на код-ревью без нового коммита). Прочитан полный текст +`src/version-recovery.ts` (не только диф) и вызывающий код в `src/houseplan-card.ts` +(`disconnectedCallback`, `requestUpdate` override, `_dangerConfirmController`, +`_clearRoomFocus`) — чтобы независимо проверить риск №1 из ТЗ («вызов `requestUpdate()` из +`disconnectedCallback` — путь не новый»), а не поверить автору на слово: `_cancelDangerConfirm()` +уже сегодня триггерит реактивный сеттер (`_dangerConfirm = state`) из того же +`disconnectedCallback`, до `super.disconnectedCallback()` — тот же порядок вызовов, тот же +путь через Lit. Прецедент подтверждён чтением, не с чужих слов. + +Диф прогнан построчно вручную (см. таблицу AC ниже) — трассировка обоих новых тестов +через `_reconcile()`/`_showBanner()`/`disconnect()` шаг за шагом с константами по умолчанию +из `harness()`/`update()`, результат совпал с зафиксированными в тесте ожиданиями. + +### Гейты — что прогнано и почему остальное не прогонялось + +| Гейт | Статус | Источник | +|---|---|---| +| `typecheck` / `test` / `build` (+ сверка 3 копий бандла) | **не перегонял** | Validate на точном SHA `1e1880f0` завершился `success` (ссылка в постановке раунда); дешёвые гейты сошлись на этом прогоне | +| сверка `dist/` ↔ `custom_components/houseplan/frontend/` | **прогнал сам** | `cmp` трёх файлов (`houseplan-card.js`, `houseplan-panel.js`, `houseplan-assets.json`) — побайтово совпадают | +| `node scripts/check-docs.mjs` | не перегонял отдельно, но факт зафиксирован | diff трогает `src/**` → обязателен; автор прогнал `npm run docs:accept -- --identical` (11 кадров совпали попиксельно, обновлён только отпечаток исходников) — засчитано, `check-docs` в Validate тоже входит в тот же зелёный прогон | +| `node scripts/mutation-gate.mjs --id=version-banner-disconnect-silent` | **не перегонял**, но проверил логику мутанта чтением | патч мутанта откатывает ровно добавленную строку `this.hooks.changed()`; заявленный автором результат «1 из 1» правдоподобен и я independently проследил, что это красит именно новый тест «notice dropped», а не оба | +| `node demo/smoke_version_recovery.mjs` | не перегонял | заявлен зелёным автором; диф не меняет DOM/рендер, только счётчик перерисовок на уровне контроллера — смок этого не видит по конструкции (ниже) | +| `node scripts/model-invariants.mjs` | не применим | diff не трогает геометрию, `layout`, толщину, `open_spans` | +| `python -m pytest tests_backend` | не применим | Python не тронут | +| `golden:verify` | не применим | диф не меняет визуальный результат (только внутренняя логика запроса перерисовки, не сама разметка) | +| перф-профили | не применим | не названы в AC, файлы вне `src/iso-*`/`src/live-*`/`src/render-*` | +| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | приложен автором | вывод: «НЕОПРЕДЕЛЁННОСТЬ: дифф исполняемый, но ни один смок не связан доказуемо», символ `_banner` не встречается ни в одном смоке — согласен с decision автора не прогонять широкий набор: `_banner` приватное поле, наблюдаемый факт («попросили ли перерисовку») не проявляется в DOM браузерного смока, там его снимает любая посторонняя перерисовка — то самое совпадение, которое чинит задача | + +Дешёвые гейты не перегонялись сознательно, а не пропущены молча: Validate на точном SHA +материала зелёный (см. ссылку в постановке раунда), сверка бандла — прогнана мной лично. + +## AC · чем доказан · чем краснеет + +| AC | Доказательство | Проверка ревьюером | +|---|---|---| +| **AC1** — последовательность «несовпадение → disconnect → версии сошлись → connect» просит ровно 2 перерисовки, плашка пуста в конце | тест `#536 a notice dropped on disconnect asks the host to repaint` | Прослежен построчно: `update({reducedMotion:false})` до `connect()` не триггерит `changed()` (не подключён → `_hideBanner()` на пустом поле — no-op); `connect()` → mismatch → `_showBanner` → `changed()` #1, `banner.phase==='visible'`; `disconnect()` → banner не пуст → `changed()` #2, banner=null; `update({backendVersion:'1.72.0'})` → relation `equal`, не подключён → no-op; `connect()` → relation `equal` (не mismatch) → `_hideBanner()` на пустом поле — no-op. Итог: 2 события `changed`, banner `null`. Совпадает с утверждениями теста. **Мутация доказана логически**: без `this.hooks.changed()` в `disconnect()` второе событие не производится, `repaints()` после `disconnect()` останется `1` — ассерт `assert.equal(repaints(), 2, …)` красный | +| **AC2** — `disconnect()` без плашки перерисовку не просит | тест `#536 a disconnect without a notice asks for nothing` | Прослежен: `update({backendVersion:'1.72.0'})` (frontend по умолчанию `1.72.0`) → relation `equal`; `connect()` → не mismatch → `_hideBanner()` на пустом поле — banner остаётся `null`, `changed()` не звался; `disconnect()` → `if (this._banner)` ложно → тело условия не выполняется, `changed()` не звался. `before === after`. **Обратная мутация** (безусловный `changed()` в `disconnect()`) красит именно этот тест — подтверждено чтением условия `if (this._banner) { …; this.hooks.changed(); }`: без охраны событие добавилось бы в любом случае | +| **AC3** — прежние утверждения контроллера не сдвинулись (таймеры, токены, kiosk-путь, reduced motion) | 11 существующих тестов `test/version-recovery.test.mjs`, ни один не тронут | Диф `test/version-recovery.test.mjs` — только добавление (`+39`, `0` изменений в существующих строках), подтверждено `git show` построчно. Логика `disconnect()` вне добавленного `if`-блока идентична коду `origin/dev` (сверено `git show origin/dev:src/version-recovery.ts` против диффа) | + +Все три AC доказаны либо тестом с прослеженной логикой мутации, либо построчным чтением +диффа против базы. Third-column (`чем краснеет`) для AC1/AC2 — не голое «tест умеет +падать», а конкретно прослеженная причинно-следственная связь между снятой строкой и +конкретным упавшим ассертом; для AC3 — «проверено чтением, не исполнением» в буквальном +смысле §2.7. + +## Проверено и корректно + +- Правка ограничена ровно объявленной строкой (`disconnect()` в + `src/version-recovery.ts:213-216`); `_hideBanner()`, `_reconcile()`, kiosk-таймер, + `claimReloadTarget`, токены анимации — не тронуты (сверено построчно с `origin/dev`). +- Комментарий рядом с правкой объясняет причину («Lit renders neither on disconnect nor on + reconnect») — не голословно: это тот же факт, что и в докстринге модуля и в постановке + issue (замер со счётчиком на собранном модуле). +- Прецедент «`requestUpdate()`/реактивный сеттер из `disconnectedCallback` не ломается» — + независимо подтверждён чтением `_cancelDangerConfirm()` → `HpConfirmController.cancel()` + → `resolve()` → `this._changed(null)` → `this._dangerConfirm = state`, вызываемого из + `disconnectedCallback` (`houseplan-card.ts:2743-2751`) **до** `super.disconnectedCallback()` + — тот же порядок, тот же путь, работает уже сегодня. +- Три копии бандла (`dist/`, `custom_components/houseplan/frontend/`) побайтово совпадают + (проверено `cmp` лично, не с чужих слов). +- Кажущийся большой диф сгенерированных чанков (`dist/houseplan-assets/*`, переименования + хешей, добавление/удаление `guard-*.js`) — проверен построчно на образцах + (`iso-scene-render`, `guard-*`): единственные реальные отличия — встроенная константа + `__HOUSEPLAN_BUILD_FINGERPRINT__` (считается по всему `src/**`, поэтому меняется от + любой правки фронтенда) и путь импорта на переименованный главный чанк + (`houseplan-card-B09T8YNT.js` → `houseplan-card-kxu4PK0e.js`, чьё содержимое реально + изменилось из-за правки `version-recovery.ts`, находящегося в eager-графе). Каскад + переименований механический, не посторонняя правка. +- `User-Visible: no` и лёгкий трек — решения, принятые на зелёном ревью ТЗ r1 + (`docs/reviews/SPEC-REVIEW-536-r1.md`), код им не противоречит: ни одно уже + наблюдавшееся поведение не изменилось (это и есть предмет AC3), changelog не тронут — + корректно для этого случая. +- Трейлеры коммита корректны: `Issue: #536`, `User-Visible: no`, ровно один коммит. +- «Одно число — один источник»: неприменимо, диф не добавляет и не меняет ни одной + величины, видимой пользователю. + +## Находки + +Нет High. Нет Medium. Нет Low сверх уже снятых на ревью ТЗ (термин «контроллер» в +формулировке «До/После» и невыписанные явно строки DoR — оба сняты решением ревьюера ТЗ +без возврата, см. `SPEC-REVIEW-536-r1.md`; повторно не поднимаю, так как это тот же +раунд документации, не код). + +## Чего не проверял + +- `typecheck`/`test`/`build` лично не перегонял — доверился зелёному Validate на точном + SHA материала (ссылка в постановке раунда), сверил только производный артефакт (бандл). +- `mutation-gate.mjs` и `demo/smoke_version_recovery.mjs` не перегонял руками — принял + заявленные автором результаты, но независимо проверил логику первого чтением кода + мутации (см. таблицу AC) вместо слепого доверия. +- Не проверял поведение в реальном браузере (ручного тестирования в цикле нет по + процессу); визуальный сценарий не пострадал бы в любом случае, так как диф не меняет + разметку/стили. + +## Материал раунда + +- SHA: `1e1880f010495fe23197feddc64e33138445fc5d` +- Дерево: единственный коммит в диапазоне `origin/dev..HEAD` +- База: `origin/dev` = `05a2c68d` + +--- + +**Вердикт: зелёный · заход r1 · блокирующих циклов 0/2 · High: 0 · Medium: 0 → в задаче** + +--- + + + +## Материал раунда + +- Ветка: `issue/536-banner-notify`, коммит `1e1880f01049` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `8d2cba9fc9bd684598fa246f90c09485b914e070` + ``` + git log --all --format='%H %T' | grep 8d2cba9fc9bd + ``` +- Тело issue: `fdc4c4f20bae1574a65b8c8bac06c43f4ecdab6d3394149baea09c34bec75cc4` +- Вердикт конвейера: `green` · High 0