mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
claude[bot]
@@ -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 → в задаче**
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/536-banner-notify`, коммит `1e1880f01049` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `8d2cba9fc9bd684598fa246f90c09485b914e070`
|
||||
```
|
||||
git log --all --format='%H %T' | grep 8d2cba9fc9bd
|
||||
```
|
||||
- Тело issue: `fdc4c4f20bae1574a65b8c8bac06c43f4ecdab6d3394149baea09c34bec75cc4`
|
||||
- Вердикт конвейера: `green` · High 0
|
||||
Reference in New Issue
Block a user